Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330593005:
HTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA (media eng… (Closed)
|
Created:
6 years, 6 months ago by Srirama Modified:
6 years, 5 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionHTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA (media engine error case)
BUG=382721
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176993
Patch Set 1 #Patch Set 2 : Fixed review comments #Patch Set 3 : Added test case #Patch Set 4 : Fixed review comments #
Total comments: 2
Patch Set 5 : Removed emptied event #
Total comments: 16
Patch Set 6 : Fixed review comments #
Total comments: 2
Patch Set 7 : updated title of the test case #Patch Set 8 : updated as per comments #Patch Set 9 : Updated failing mediasource test #
Total comments: 8
Patch Set 10 : updated as per review comments #
Messages
Total messages: 34 (0 generated)
This is one of the cases where webMediaPlayer() will be null when m_readyState >= HAVE_METADATA. This will come because the media engine error handling is not as per the spec. There is a separate handling or network error and decode error in step4. http://www.w3.org/TR/2011/WD-html5-20110113/video.html#media-elements Please check the step4 under the sections a) If the connection is interrupted, causing the user agent to give up trying to fetch the resource b) If the media data is corrupted Modified mediaEngineError() function as per the spec.
Since mediaEngineError() implements two algorithms at once, I think it would be slightly better to pass in the MediaError::Code and use that to both MediaError::create() and to switch between the two different step 4s. AFAICT, the existing code doesn't match either of the two algorithms in the spec, which says "Set the element's networkState attribute to the NETWORK_IDLE value." for network errors and "If the media element's readyState attribute has a value equal to HAVE_NOTHING, set the element's networkState attribute to the NETWORK_EMPTY value, set the element's show poster flag to true, and fire a simple event named emptied at the element." for decode errors. This also needs tests.
Updated code as per review comments, will check on test cases part.
On 2014/06/13 14:15:55, philipj wrote: > Since mediaEngineError() implements two algorithms at once, I think it would be > slightly better to pass in the MediaError::Code and use that to both > MediaError::create() and to switch between the two different step 4s. > Done > AFAICT, the existing code doesn't match either of the two algorithms in the > spec, which says "Set the element's networkState attribute to the NETWORK_IDLE > value." for network errors and "If the media element's readyState attribute has > a value equal to HAVE_NOTHING, set the element's networkState attribute to the > NETWORK_EMPTY value, set the element's show poster flag to true, and fire a > simple event named emptied at the element." for decode errors. > Yes, the above two cases were not handled in the current code. I have handled both cases. I didn't modify the show poster flag part as it will be handled by updateDisplayState() in mediaLoadingFailed() > This also needs tests. I have a partial test case to simulate the issue but i have to manually interrupt the network connection. Any suggestions on simulating the network and decode errors during video play? Is it something like reading the video file using javascript and abort it after loadedmetadata event?
On 2014/06/13 15:02:31, Srirama wrote: > On 2014/06/13 14:15:55, philipj wrote: > > Since mediaEngineError() implements two algorithms at once, I think it would > be > > slightly better to pass in the MediaError::Code and use that to both > > MediaError::create() and to switch between the two different step 4s. > > Done > > AFAICT, the existing code doesn't match either of the two algorithms in the > > spec, which says "Set the element's networkState attribute to the NETWORK_IDLE > > value." for network errors and "If the media element's readyState attribute > has > > a value equal to HAVE_NOTHING, set the element's networkState attribute to the > > NETWORK_EMPTY value, set the element's show poster flag to true, and fire a > > simple event named emptied at the element." for decode errors. > > Yes, the above two cases were not handled in the current code. I have handled > both cases. > I didn't modify the show poster flag part as it will be handled by > updateDisplayState() in mediaLoadingFailed() > > This also needs tests. > I have a partial test case to simulate the issue but i have to manually > interrupt the network connection. > Any suggestions on simulating the network and decode errors during video play? > Is it something like reading the video file using javascript and abort it after > loadedmetadata event? Take a look in LayoutTests/http/tests/media . You should be able to create something like video-throttled-load.cgi that either drops the connection early or corrupts the data so an error occurs.
On 2014/06/13 17:11:51, acolwell wrote: > On 2014/06/13 15:02:31, Srirama wrote: > > On 2014/06/13 14:15:55, philipj wrote: > > > Since mediaEngineError() implements two algorithms at once, I think it would > > be > > > slightly better to pass in the MediaError::Code and use that to both > > > MediaError::create() and to switch between the two different step 4s. > > > Done > > > AFAICT, the existing code doesn't match either of the two algorithms in the > > > spec, which says "Set the element's networkState attribute to the > NETWORK_IDLE > > > value." for network errors and "If the media element's readyState attribute > > has > > > a value equal to HAVE_NOTHING, set the element's networkState attribute to > the > > > NETWORK_EMPTY value, set the element's show poster flag to true, and fire a > > > simple event named emptied at the element." for decode errors. > > > Yes, the above two cases were not handled in the current code. I have > handled > > both cases. > > I didn't modify the show poster flag part as it will be handled by > > updateDisplayState() in mediaLoadingFailed() > > > This also needs tests. > > I have a partial test case to simulate the issue but i have to manually > > interrupt the network connection. > > Any suggestions on simulating the network and decode errors during video play? > > Is it something like reading the video file using javascript and abort it > after > > loadedmetadata event? > > Take a look in LayoutTests/http/tests/media . You should be able to create > something like video-throttled-load.cgi that either drops the connection early > or corrupts the data so an error occurs. Thanks, I will check it.
Added test case. Tried so hard and finally able to write a test for decode error. Not able to get network error, even if i send less number of bytes it shows decode error only. PTAL.
While reviewing this I noticed that what the spec currently says doesn't actually make sense: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26133 All that would remain if that bug is fixed is "m_networkState = NETWORK_IDLE" for both cases. Note that for some reason HTMLMediaElement::mediaLoadingFailed doesn't have the m_readyState >= HAVE_METADATA check for the decode case, which it should per spec. Maybe it's always true (in which case it can be asserted) or it's just wrong. Since any fix here is going to change behavior, I'd like to await the conclusion of the bug first. The title of this CL is "HTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA". Is there some smaller change that can be landed to make that true that doesn't change observable behavior?
@philipj: you are right, the condition m_readyState == HAVE_NOTHING will never come in mediaEngineError(). I have checked webmediaplayer_impl and webmediaplayer_android, both convert decode errors to networkformat error if the readystate is HAVE_NOTHING. So as discussed i have just added an assert and set networkstate to NETWORK_IDLE to solve the problem of "webmediaplayer() being null when readystate is >= HAVE_METADATA". Lets wait for acolwell to comment.
Looks good to me, although there is some risk that Hixie will fix it some other way, in which case this would have to change again. https://codereview.chromium.org/330593005/diff/130001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/330593005/diff/130001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1481: scheduleEvent(EventTypeNames::emptied); If we are to match what the spec will probably come to say, it should be "Set the element's networkState attribute to the NETWORK_IDLE value." In other words, no emptied event here.
Removed the unnecessary emptied event. https://codereview.chromium.org/330593005/diff/130001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/330593005/diff/130001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1481: scheduleEvent(EventTypeNames::emptied); On 2014/06/18 13:52:14, philipj wrote: > If we are to match what the spec will probably come to say, it should be "Set > the element's networkState attribute to the NETWORK_IDLE value." > > In other words, no emptied event here. Done.
I suspect that these changes will require updating some tests, so I started the try bots for you. You probably want to wait for acolwell's word though, since it's not a given that we should make this change just yet, even if I think it's reasonable.
Huh, the bots looks happy. I guess there was no test coverage, then :/
On 2014/06/18 15:27:36, philipj wrote: > Huh, the bots looks happy. I guess there was no test coverage, then :/ yes, there is no test coverage and that is the reason when i submitted my previous patches (Eliminate MediaPlayer & MediaPlayerClient abstractions) it created some crashes.
https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi (right): https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:14: my $type = $query->param('type') || "video/mp4"; nit: Please remove the default so this parameter must be specified. Could a 400 error be returned if this parameter is not provided? https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:19: print "Cache-Control: no-cache\n"; What is responsible for printing the HTTP status line? https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.html (right): https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:14: logResult(true, "failed to load media file"); nit: Just use consoleWrite() here and above since a condition isn't actually being tested in either place. https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:15: testExpected("video.networkState", 1); s/1/HTMLMediaElement.NETWORK_IDLE to make it clearer what is expected. https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:28: video.src = "http://127.0.0.1:8000/media/video-load-metadata-decode-error.cgi?name=" + movie + "&type=" + type; nit: Do you really need the scheme and host? Will the following relative path work? "video-load-metadata-decode-error.cgi?name=" + movie + "&type=" + ty https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:30: video.play(); You should only need load() or play(). I don't think you should need both. Ideally it would be nice if you could just use load(). https://codereview.chromium.org/330593005/diff/170001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/330593005/diff/170001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1477: closeMediaSource(); This line should be removed if we aren't actually transitioning to NETWORK_EMPTY now.
https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi (right): https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:14: my $type = $query->param('type') || "video/mp4"; I will remove the default parameter. But I feel we should not return 400 because our test expects atleast meta data to load and then give some error. On 2014/06/18 23:46:27, acolwell wrote: > nit: Please remove the default so this parameter must be specified. Could a 400 > error be returned if this parameter is not provided? https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:19: print "Cache-Control: no-cache\n"; I think based on the CGI output apache server will generate the http status line. As long as there is no error in CGI execution it appends 200 status. On 2014/06/18 23:46:27, acolwell wrote: > What is responsible for printing the HTTP status line? https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.html (right): https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:14: logResult(true, "failed to load media file"); will do it. On 2014/06/18 23:46:27, acolwell wrote: > nit: Just use consoleWrite() here and above since a condition isn't actually > being tested in either place. https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:15: testExpected("video.networkState", 1); will do it. On 2014/06/18 23:46:27, acolwell wrote: > s/1/HTMLMediaElement.NETWORK_IDLE to make it clearer what is expected. https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:28: video.src = "http://127.0.0.1:8000/media/video-load-metadata-decode-error.cgi?name=" + movie + "&type=" + type; May not be required, will check and remove. On 2014/06/18 23:46:27, acolwell wrote: > nit: Do you really need the scheme and host? Will the following relative path > work? > "video-load-metadata-decode-error.cgi?name=" + movie + "&type=" + ty https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:30: video.play(); I think there is a problem without play(). May be metadata is missing or error is not coming. Will check and remove load(). On 2014/06/18 23:46:27, acolwell wrote: > You should only need load() or play(). I don't think you should need both. > Ideally it would be nice if you could just use load(). https://codereview.chromium.org/330593005/diff/170001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/330593005/diff/170001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1477: closeMediaSource(); will do it. On 2014/06/18 23:46:27, acolwell wrote: > This line should be removed if we aren't actually transitioning to NETWORK_EMPTY > now.
Updated as per review comments. PTAL
https://codereview.chromium.org/330593005/diff/230001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.html (right): https://codereview.chromium.org/330593005/diff/230001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:3: <title>throttled loading metadata</title> I think you forgot to update the title.
Thanks for pointing :), updated the title now. https://codereview.chromium.org/330593005/diff/230001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.html (right): https://codereview.chromium.org/330593005/diff/230001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.html:3: <title>throttled loading metadata</title> On 2014/06/19 13:28:38, philipj wrote: > I think you forgot to update the title. Done.
PTAL
lgtm % comment https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi (right): https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:14: my $type = $query->param('type') || "video/mp4"; On 2014/06/19 03:35:15, Srirama wrote: > I will remove the default parameter. But I feel we should not return 400 because > our test expects atleast meta data to load and then give some error. > On 2014/06/18 23:46:27, acolwell wrote: > > nit: Please remove the default so this parameter must be specified. Could a > 400 > > error be returned if this parameter is not provided? > But returning 400 will likely cause a test to fail if the type parameter is not specified. That failure would then prompt someone to properly specify the type parameter in the URL. You should be able to add something like the following. if (!$type) { print "Status: 400 Bad Request\r\n"; return; }
On 2014/06/23 16:35:28, acolwell wrote: > lgtm % comment > > https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... > File LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi (right): > > https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... > LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:14: my $type = > $query->param('type') || "video/mp4"; > On 2014/06/19 03:35:15, Srirama wrote: > > I will remove the default parameter. But I feel we should not return 400 > because > > our test expects atleast meta data to load and then give some error. > > On 2014/06/18 23:46:27, acolwell wrote: > > > nit: Please remove the default so this parameter must be specified. Could a > > 400 > > > error be returned if this parameter is not provided? > > > But returning 400 will likely cause a test to fail if the type parameter is not > specified. That failure would then prompt someone to properly specify the type > parameter in the URL. > > You should be able to add something like the following. > if (!$type) { > print "Status: 400 Bad Request\r\n"; > return; > } sure, will do that, thanks for the suggestion
updated with http status line incase video type is missing. https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi (right): https://codereview.chromium.org/330593005/diff/170001/LayoutTests/http/tests/... LayoutTests/http/tests/media/video-load-metadata-decode-error.cgi:14: my $type = $query->param('type') || "video/mp4"; On 2014/06/23 16:35:28, acolwell wrote: > On 2014/06/19 03:35:15, Srirama wrote: > > I will remove the default parameter. But I feel we should not return 400 > because > > our test expects atleast meta data to load and then give some error. > > On 2014/06/18 23:46:27, acolwell wrote: > > > nit: Please remove the default so this parameter must be specified. Could a > > 400 > > > error be returned if this parameter is not provided? > > > But returning 400 will likely cause a test to fail if the type parameter is not > specified. That failure would then prompt someone to properly specify the type > parameter in the URL. > > You should be able to add something like the following. > if (!$type) { > print "Status: 400 Bad Request\r\n"; > return; > } Done.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/330593005/270001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12494) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13250) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Updated the test case mediasource-errors.html as mediasource is not closed on decode/network error. PTAL
lgtm % comments https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/media-source/mediasource-errors.html (left): https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:107: test.expectEvent(mediaSource, "sourceended", "mediaSource ended."); This line should not be removed. The sourceended event should fire because endOfStream() is being called below. https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:119: assert_equals(mediaSource.readyState, "closed"); This should be changed to check for "ended". https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:138: test.expectEvent(mediaSource, "sourceended", "mediaSource ended."); ditto. https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:150: assert_equals(mediaSource.readyState, "closed"); ditto
Updated before patch submission. https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... File LayoutTests/http/tests/media/media-source/mediasource-errors.html (left): https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:107: test.expectEvent(mediaSource, "sourceended", "mediaSource ended."); On 2014/06/25 16:20:07, acolwell wrote: > This line should not be removed. The sourceended event should fire because > endOfStream() is being called below. Done. https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:119: assert_equals(mediaSource.readyState, "closed"); On 2014/06/25 16:20:07, acolwell wrote: > This should be changed to check for "ended". Done. https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:138: test.expectEvent(mediaSource, "sourceended", "mediaSource ended."); On 2014/06/25 16:20:07, acolwell wrote: > ditto. Done. https://codereview.chromium.org/330593005/diff/290001/LayoutTests/http/tests/... LayoutTests/http/tests/media/media-source/mediasource-errors.html:150: assert_equals(mediaSource.readyState, "closed"); On 2014/06/25 16:20:07, acolwell wrote: > ditto Done.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srirama.m@samsung.com/330593005/310001
Message was sent while issue was closed.
Change committed as 176993 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
