Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
740663002:
Relanding 'Ignore seek operations to the current time in pause state' patch (Closed)
DescriptionIgnore seek operations to the current time during pause state. This change
is required to remove the noseekrequired logic in HTMLMediaElement.cpp
BUG=266631
Committed: https://crrev.com/36ab2684f2422d6cb49c2462333816fb9857cfe6
Cr-Commit-Position: refs/heads/master@{#307857}
Patch Set 1 : Fixed issue with paused time #
Total comments: 6
Patch Set 2 : Fixed review comments #Patch Set 3 : Fix duration against current_time on ended callback #Patch Set 4 : Fixed crashes with test cases #Patch Set 5 : Removed duration change callback #
Total comments: 3
Patch Set 6 : Test for bot results #Patch Set 7 : Pretend currenttime as duration if it's a few ms less #
Total comments: 4
Patch Set 8 : Fixed issue with durationevent call in pipeline_unittest #Patch Set 9 : Simplified GetMediaTime() as per review #
Total comments: 11
Patch Set 10 : Fixed review comments #
Total comments: 3
Messages
Total messages: 71 (16 generated)
Patchset #4 (id:60001) has been deleted
Patchset #8 (id:160001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #1 (id:80001) has been deleted
srirama.m@samsung.com changed reviewers: + dalecurtis@chromium.org, philipj@opera.com
This previous patch has been reverted in https://codereview.chromium.org/726523003/ because the test MediaTest.VideoBear3gpAacH264/0 was failing (timing out). Luckily i was able to simulate the issue with linux_chromium_chrome_rel bot, not sure how it was missed when the patch was actually landed. So i did a bit of investigation with the help of logs with the linux_chromium_chrome_rel bot. I found out that the issue is actually with the pipeline_.GetMediaTime() in pause() function in webmediaplayer_impl. In the test case there is a function SeekTestStep() which is invoked on 'ended' event. In this function there is a seek to 0.9*duration value. So ideally it should not be the "seek to sametime" scenario but it is happening that way because of wrong value from pipeline_.GetMediaTime() which is returning a value less than the duration even though playback has been completed. So I have used pipeline_.GetMediaDuration() instead of pipeline_.GetMediaTime(), in pause function when the playback has ended. PTAL.
https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:306: // FIXME: In some cases GetMediaTime() returns a value less than duration Would it also fix the problem to always clamp currentTime to duration or alternatively to report a new duration when currentTime exceeds it? It seems like inconsistency could be observable before ended_ is true.
https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:306: // FIXME: In some cases GetMediaTime() returns a value less than duration On 2014/11/20 15:27:21, philipj wrote: > Would it also fix the problem to always clamp currentTime to duration or > alternatively to report a new duration when currentTime exceeds it? It seems > like inconsistency could be observable before ended_ is true. Instead, can you try modifying GetMediaTime() in media/base/pipeline.cc to return duration_ if renderer_ended_ is true?
https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:306: // FIXME: In some cases GetMediaTime() returns a value less than duration On 2014/11/20 19:02:27, DaleCurtis wrote: > On 2014/11/20 15:27:21, philipj wrote: > > Would it also fix the problem to always clamp currentTime to duration or > > alternatively to report a new duration when currentTime exceeds it? It seems > > like inconsistency could be observable before ended_ is true. > > Instead, can you try modifying GetMediaTime() in media/base/pipeline.cc to > return duration_ if renderer_ended_ is true? Wouldn't that have the same problem, that currentTime can exceed duration until the real end is reached? That'll be timing sensitive, so any bugs related to the inconsistency may become harder to track down.
https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:306: // FIXME: In some cases GetMediaTime() returns a value less than duration On 2014/11/20 19:10:43, philipj wrote: > On 2014/11/20 19:02:27, DaleCurtis wrote: > > On 2014/11/20 15:27:21, philipj wrote: > > > Would it also fix the problem to always clamp currentTime to duration or > > > alternatively to report a new duration when currentTime exceeds it? It seems > > > like inconsistency could be observable before ended_ is true. > > > > Instead, can you try modifying GetMediaTime() in media/base/pipeline.cc to > > return duration_ if renderer_ended_ is true? > > Wouldn't that have the same problem, that currentTime can exceed duration until > the real end is reached? That'll be timing sensitive, so any bugs related to the > inconsistency may become harder to track down. Pipeline already clamps currentTime to duration if it's over.
https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:306: // FIXME: In some cases GetMediaTime() returns a value less than duration On 2014/11/20 19:14:39, DaleCurtis wrote: > On 2014/11/20 19:10:43, philipj wrote: > > On 2014/11/20 19:02:27, DaleCurtis wrote: > > > On 2014/11/20 15:27:21, philipj wrote: > > > > Would it also fix the problem to always clamp currentTime to duration or > > > > alternatively to report a new duration when currentTime exceeds it? It > seems > > > > like inconsistency could be observable before ended_ is true. > > > > > > Instead, can you try modifying GetMediaTime() in media/base/pipeline.cc to > > > return duration_ if renderer_ended_ is true? > > > > Wouldn't that have the same problem, that currentTime can exceed duration > until > > the real end is reached? That'll be timing sensitive, so any bugs related to > the > > inconsistency may become harder to track down. > > Pipeline already clamps currentTime to duration if it's over. D'oh! If I had read the comment carefully, I would have seen that this is about when currentTime stops short of duration, not when it goes beyond. Then the options are claiming that currentTime is duration, or reporting a new duration. If there is no limit to how wrong the previously reported duration can be, I think reporting a new duration seems like the sane thing.
https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/740663002/diff/180001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:306: // FIXME: In some cases GetMediaTime() returns a value less than duration On 2014/11/21 00:26:38, philipj wrote: > On 2014/11/20 19:14:39, DaleCurtis wrote: > > On 2014/11/20 19:10:43, philipj wrote: > > > On 2014/11/20 19:02:27, DaleCurtis wrote: > > > > On 2014/11/20 15:27:21, philipj wrote: > > > > > Would it also fix the problem to always clamp currentTime to duration or > > > > > alternatively to report a new duration when currentTime exceeds it? It > > seems > > > > > like inconsistency could be observable before ended_ is true. > > > > > > > > Instead, can you try modifying GetMediaTime() in media/base/pipeline.cc to > > > > return duration_ if renderer_ended_ is true? > > > > > > Wouldn't that have the same problem, that currentTime can exceed duration > > until > > > the real end is reached? That'll be timing sensitive, so any bugs related to > > the > > > inconsistency may become harder to track down. > > > > Pipeline already clamps currentTime to duration if it's over. > > D'oh! If I had read the comment carefully, I would have seen that this is about > when currentTime stops short of duration, not when it goes beyond. > > Then the options are claiming that currentTime is duration, or reporting a new > duration. If there is no limit to how wrong the previously reported duration can > be, I think reporting a new duration seems like the sane thing. But here we are trying to correct/manipulate the current time (paused_time_) and not the duration right. And it is the specific case of "Pipeline End", so duration would have been finalized and reported even though we have reported wrong duration before. Did i misunderstood something?
Patchset #2 (id:200001) has been deleted
Isn't the root cause of all of this that the initially reported duration may not be the same as currentTime after playback has ended? Dale says that the pipeline already clamps currentTime to duration if it's over, and I'm thinking of a similar fix for the case where it stops short. Might as well fix the whole problem instead of just when paused and ended, I'm thinking.
On 2014/11/21 08:24:22, philipj wrote: > Isn't the root cause of all of this that the initially reported duration may not > be the same as currentTime after playback has ended? Dale says that the pipeline > already clamps currentTime to duration if it's over, and I'm thinking of a > similar fix for the case where it stops short. Might as well fix the whole > problem instead of just when paused and ended, I'm thinking. ok, lets see what Dale says.
Hmm, I could see updating the duration during the ended event as well as when currentTime moves past the current duration. I.e. - If currentTime > duration during GetMediaTime(), update duration to currentTime. - If upon ended currentTime < duration, update duration to currentTime. I don't know if that will cause problems in HTMLMediaElement though. I also think those changes should probably be in another CL though since I suspect expectations will change for several tests. WDYT Philip?
Assuming there is no limit to how wrong the initially known duration can be, I think updating the duration when it turns out to be wrong is the least magical thing we can do. However, that is a bit of a risky change in itself, and I would not be opposed to a bit of a workaround to finally get rid of the noSeekRequired logic in HTMLMediaElement. The case where currentTime stops short of duration is the simpler case, and the one relevant to this change. How about reporting a new duration playback ends if currentTime is less than duration, and leaving the other case until a later CL?
That sgtm.
Updated as discussed. There is a failure in android_dbg_tests_recipe in patchset-3, as there is a call for durationchange twice. Please suggest me if i have to change the test, or i have to modify my logic.
https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc#... media/base/pipeline.cc:640: duration_ = media_time; Shouldn't this be SetDuration(media_time), or what will end up invoking duration_change_cb_?
https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc#... media/base/pipeline.cc:640: duration_ = media_time; On 2014/11/25 10:01:04, philipj wrote: > Shouldn't this be SetDuration(media_time), or what will end up invoking > duration_change_cb_? I have done a similar thing in patchset4, but there are many failures as the duration change callback is invoked twice where as it is expected only once. So do we need to fix those failures or directly calling SetDuration() will not result in those failures? I created patchset5 by removing callback for testing to see the results in all the bots.
https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc#... media/base/pipeline.cc:640: duration_ = media_time; On 2014/11/25 10:19:29, Srirama wrote: > On 2014/11/25 10:01:04, philipj wrote: > > Shouldn't this be SetDuration(media_time), or what will end up invoking > > duration_change_cb_? > > I have done a similar thing in patchset4, but there are many failures as the > duration change callback is invoked twice where as it is expected only once. So > do we need to fix those failures or directly calling SetDuration() will not > result in those failures? > > I created patchset5 by removing callback for testing to see the results in all > the bots. By only setting duration_, I guess HTMLMediaElement.duration will change without a durationchange event being fired? That's not OK, so I think updating tests to deal with the new durationchange event is the only way to go. Of course this change may have introduced real bugs, so check carefully :)
On 2014/11/25 12:24:29, philipj wrote: > https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc > File media/base/pipeline.cc (right): > > https://codereview.chromium.org/740663002/diff/280001/media/base/pipeline.cc#... > media/base/pipeline.cc:640: duration_ = media_time; > On 2014/11/25 10:19:29, Srirama wrote: > > On 2014/11/25 10:01:04, philipj wrote: > > > Shouldn't this be SetDuration(media_time), or what will end up invoking > > > duration_change_cb_? > > > > I have done a similar thing in patchset4, but there are many failures as the > > duration change callback is invoked twice where as it is expected only once. > So > > do we need to fix those failures or directly calling SetDuration() will not > > result in those failures? > > > > I created patchset5 by removing callback for testing to see the results in all > > the bots. > > By only setting duration_, I guess HTMLMediaElement.duration will change without > a durationchange event being fired? That's not OK, so I think updating tests to > deal with the new durationchange event is the only way to go. Of course this > change may have introduced real bugs, so check carefully :) Ok, so i will do the following 1) Firing of durationchange twice is fine and i will modify the related test cases 2) Modify test cases depending on durationchange event 3) Check other failed test cases which are affected because of this change and fix them or modify them
Yes, two durationchange events will be fine, as long as one is just before the loadedmetadata event and the other is just before the ended event. A LayoutTest verifying the order of durationchange and ended would be good.
On 2014/11/25 13:10:43, philipj wrote: > Yes, two durationchange events will be fine, as long as one is just before the > loadedmetadata event and the other is just before the ended event. A LayoutTest > verifying the order of durationchange and ended would be good. Thanks, will do it, so can i land the test case as a separate CL before this or should i wait for this and then land it?
If you prepare the Blink-side test and verifying that it fails without this CL and passes with it, landing it after would result in the least amount of churn I think.
On 2014/11/25 16:05:21, philipj wrote: > If you prepare the Blink-side test and verifying that it fails without this CL > and passes with it, landing it after would result in the least amount of churn I > think. ok, i will do that.
As i understand i need to create a media file with wrong meta data which reports a duration value little more than the actual duration of the media file. I verified some of the test cases in media layout tests and when i try to do a seek to almost end of file manually they report a current time which is less than duration (here i assume duration to be correct because when i play normally it reports duration and current time same). So in our previous discussions we have decided to update duration to current time but the above test cases prove that there are cases where current time is wrong when current time falls short of duration. So are we going in the right direction?
How can currentTime be wrong, though? Is it reporting some other value after seeking than it would have if one played through normally up to that point? I could imagine that happening for VBR MP3 without the appropriate metadata where only decoding the whole file allows the accurate duration to be reported, but is that what we're talking about? As for producing that test file, the easiest way may be to start with a valid file and cut off the end of it at an offset exactly aligned with some container format structure, such that no decoding error results but some media data is missing.
On 2014/11/27 12:40:57, philipj wrote: > How can currentTime be wrong, though? Is it reporting some other value after > seeking than it would have if one played through normally up to that point? I > could imagine that happening for VBR MP3 without the appropriate metadata where > only decoding the whole file allows the accurate duration to be reported, but is > that what we're talking about? > > As for producing that test file, the easiest way may be to start with a valid > file and cut off the end of it at an offset exactly aligned with some container > format structure, such that no decoding error results but some media data is > missing. I have done the testing as below by opening some of the layout tests files directly in linux chrome. 1) Play a file normally till the end and see the values of media time and duration in pipeline_end event in webmediaplayer_impl. Here both the values are equal for ex: 1.5sec (so i assume 1.5 is the correct duration of the file). 2) Play the same file, and then seek to nearly end of the file, and wait for completion of the play. Now the values observed in pipeline_end event are 1.5 for duration and 1.4 for current time. So here current time is less than duration and it is wrong where as duration is correct. Isn't is opposite of what we are thinking? For the test file i will try as you suggested.
Yeah, that sounds problematic indeed. For which input files does this happen?
On 2014/11/27 13:40:55, philipj wrote: > Yeah, that sounds problematic indeed. For which input files does this happen? These are some of the files i have tested with. Though the issue is not always. media-ended.html src/content/test/data/media/bear.webm media-continues-playing-after-replace-source.html
That's odd, WebM isn't a format I would suspect of being susceptible to this kind of error, since it has a seek index and everything has timestamps. Dale, do you have any idea what might be going on?
On 2014/11/27 15:13:52, philipj wrote: > That's odd, WebM isn't a format I would suspect of being susceptible to this > kind of error, since it has a seek index and everything has timestamps. Dale, do > you have any idea what might be going on? Not only webM but counting.ogv, silence.wav have also shown similar behaviour when i tested. I have even analyzed the file counting.ogv with vorbis-tool to validate the duration value which we are getting and it is correct. So isn't it the issue exposed by the earlier failure MediaTest.VideoBear3gpAacH264/0 ?
For WAVE PCM it seems even more mysterious, since you can't really go wrong when mapping a byte offset to a time offset in that case. This seems really strange, how big is the error and can you hear that not all of the audio finishes playing? Maybe it's an audio backend thing and not a problem with determining the correct times after seeking?
On 2014/11/28 09:17:02, philipj wrote: > For WAVE PCM it seems even more mysterious, since you can't really go wrong when > mapping a byte offset to a time offset in that case. This seems really strange, > how big is the error and can you hear that not all of the audio finishes > playing? Maybe it's an audio backend thing and not a problem with determining > the correct times after seeking? It is only few micro seconds. For bear.webm case it is around 12micro seconds(Not sure if we need to worry about this error). [5155:5155:1128/163217:ERROR:webmediaplayer_impl.cc(717)] OnPipelineEnded, duration: 1001000, CurrentTime: 988764 [5155:5155:1128/163232:ERROR:webmediaplayer_impl.cc(717)] OnPipelineEnded, duration: 1001000, CurrentTime: 996654
On 2014/11/28 11:06:00, Srirama wrote: > On 2014/11/28 09:17:02, philipj wrote: > > For WAVE PCM it seems even more mysterious, since you can't really go wrong > when > > mapping a byte offset to a time offset in that case. This seems really > strange, > > how big is the error and can you hear that not all of the audio finishes > > playing? Maybe it's an audio backend thing and not a problem with determining > > the correct times after seeking? > > It is only few micro seconds. For bear.webm case it is around 12micro > seconds(Not sure if we need to worry about this error). > [5155:5155:1128/163217:ERROR:webmediaplayer_impl.cc(717)] OnPipelineEnded, > duration: 1001000, CurrentTime: 988764 > [5155:5155:1128/163232:ERROR:webmediaplayer_impl.cc(717)] OnPipelineEnded, > duration: 1001000, CurrentTime: 996654 Sorry, it is 12milli seconds.
That sounds like a bug, but not very serious. If there's a workaround that would allow the CL to proceed that would be great. Maybe if currentTime stops within 250ms of duration, pretend that it reached duration instead of triggering a durationchange event? 250ms is the maximum allowed time between timeupdate events, but still arbitrary of course.
On 2014/11/28 12:19:27, philipj wrote: > That sounds like a bug, but not very serious. If there's a workaround that would > allow the CL to proceed that would be great. Maybe if currentTime stops within > 250ms of duration, pretend that it reached duration instead of triggering a > durationchange event? 250ms is the maximum allowed time between timeupdate > events, but still arbitrary of course. Hmm, even in the previous failure case the difference was 128ms so your suggested work around will fix that. And i guess all the bot failures will be resolved and i think duration change event will never be triggered. I have uploaded a test patch, lets see what will be the results.
Patchset #7 (id:320001) has been deleted
The reported current time is based on played out audio frames, so it may be more that seeking is landing slightly after the desired seek point and thus can never advance to the real duration. Since that calculation includes a potentially unreliable hardware output delay, it may also go slightly askew... You might take a look at AudioRendererImpl to see where the calculation using AudioClock is going wrong relative to the decoded AudioBuffer's received. It's definitely an issue we should look into, but I'm not sure I follow exactly why the current patch set checks that the delta is within 250ms. What's wrong with issuing the SetDuration() during ended regardless of the delta? Then also just returning duration_ when media_time_ < duration && ended?
https://codereview.chromium.org/740663002/diff/340001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/340001/media/base/pipeline.cc#... media/base/pipeline.cc:165: // FIXME: In some cases GetMediaTime() returns a value few milli seconds In chromium code the syntax is "TODO(<your user name>): ..." not FIXME. milliseconds is one word too. https://codereview.chromium.org/740663002/diff/340001/media/base/pipeline.cc#... media/base/pipeline.cc:169: if ((duration_ - media_time).ToInternalValue() <= 250000) Note: Use InMilliseconds() not ToInternalValue().
Patchset #9 (id:380001) has been deleted
On 2014/12/01 19:32:02, DaleCurtis wrote: > The reported current time is based on played out audio frames, so it may be more > that seeking is landing slightly after the desired seek point and thus can never > advance to the real duration. Since that calculation includes a potentially > unreliable hardware output delay, it may also go slightly askew... > > You might take a look at AudioRendererImpl to see where the calculation using > AudioClock is going wrong relative to the decoded AudioBuffer's received. Sure, but can i check it after landing this one? > > It's definitely an issue we should look into, but I'm not sure I follow exactly > why the current patch set checks that the delta is within 250ms. What's wrong > with issuing the SetDuration() during ended regardless of the delta? Then also > just returning duration_ when media_time_ < duration && ended? As per discussions above between myself and philip, there are cases where duration is actually correct and currenttime just stops short of duration by a few milliseconds. So if we just do SetDuration(), it will actually change the duration to wrong value in those cases. Please suggest if you have a better way out here than 250ms, so that we can land this as soon as possible :( For the 2nd comment your point is valid and i think we don't even need media_time < duration check as media_time is anyway clamped to duration below. So just returning duration_ on ended event should be fine.
Please take a look at the final patch set (patch set 9 after several tests and fixes). https://codereview.chromium.org/740663002/diff/340001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/340001/media/base/pipeline.cc#... media/base/pipeline.cc:165: // FIXME: In some cases GetMediaTime() returns a value few milli seconds On 2014/12/01 19:33:48, DaleCurtis wrote: > In chromium code the syntax is "TODO(<your user name>): ..." not FIXME. > > milliseconds is one word too. Done. https://codereview.chromium.org/740663002/diff/340001/media/base/pipeline.cc#... media/base/pipeline.cc:169: if ((duration_ - media_time).ToInternalValue() <= 250000) On 2014/12/01 19:33:48, DaleCurtis wrote: > Note: Use InMilliseconds() not ToInternalValue(). Done. https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc#... media/base/pipeline.cc:35: const int kCurrTimeErrorInMillisecondsOnPlaybackEnd = 250; Do we need this, or can i directly use 250 below?
https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc#... media/base/pipeline.cc:35: const int kCurrTimeErrorInMillisecondsOnPlaybackEnd = 250; On 2014/12/02 11:16:29, Srirama wrote: > Do we need this, or can i directly use 250 below? Since you only have one usage, it's fine to inline it below. Please file a bug for this issue and link it in both of your comments. https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc#... media/base/pipeline.cc:651: kCurrTimeErrorInMillisecondsOnPlaybackEnd) You don't need the media_time < duration_ check since duration_ < media_time will yield a negative value. Just remove the outer conditional and inline renderer_->GetMediaTime(). https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is Hmm, doesn't this add a bunch of log spam / errors in some cases where it doesn't get triggered? https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:623: // an onDurationChange event fired again on OnEnded event if required. s/onDuration/OnDuration/
Patchset #10 (id:420001) has been deleted
https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc#... media/base/pipeline.cc:35: const int kCurrTimeErrorInMillisecondsOnPlaybackEnd = 250; On 2014/12/02 18:14:05, DaleCurtis wrote: > On 2014/12/02 11:16:29, Srirama wrote: > > Do we need this, or can i directly use 250 below? > > Since you only have one usage, it's fine to inline it below. Please file a bug > for this issue and link it in both of your comments. Done. https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline.cc#... media/base/pipeline.cc:651: kCurrTimeErrorInMillisecondsOnPlaybackEnd) On 2014/12/02 18:14:05, DaleCurtis wrote: > You don't need the media_time < duration_ check since duration_ < media_time > will yield a negative value. Just remove the outer conditional and inline > renderer_->GetMediaTime(). Removed the condition but not inlined the function as it is used in two places. https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is On 2014/12/02 18:14:05, DaleCurtis wrote: > Hmm, doesn't this add a bunch of log spam / errors in some cases where it > doesn't get triggered? I thought about it initially but after seeing the local unittest results and bot results i thought it is ok. I don't know if i can do it conditionally similar to what i am doing in RunEndedCallbackIfNeeded(). What do you suggest? https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:623: // an onDurationChange event fired again on OnEnded event if required. On 2014/12/02 18:14:05, DaleCurtis wrote: > s/onDuration/OnDuration/ Done.
https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is On 2014/12/03 14:43:02, Srirama wrote: > On 2014/12/02 18:14:05, DaleCurtis wrote: > > Hmm, doesn't this add a bunch of log spam / errors in some cases where it > > doesn't get triggered? > > I thought about it initially but after seeing the local unittest results and bot > results i thought it is ok. I don't know if i can do it conditionally similar to > what i am doing in RunEndedCallbackIfNeeded(). What do you suggest? If there's just a few tests which trigger this OnDuration change you should explicitly add it to those tests. If there's a bunch (> 5) you can consider switching to a NiceMock; though we'll want to hunt down what the real issue is here. https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is Same comment as PipelineIntegrationTest.
https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/400001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is On 2014/12/04 02:04:41, DaleCurtis wrote: > On 2014/12/03 14:43:02, Srirama wrote: > > On 2014/12/02 18:14:05, DaleCurtis wrote: > > > Hmm, doesn't this add a bunch of log spam / errors in some cases where it > > > doesn't get triggered? > > > > I thought about it initially but after seeing the local unittest results and > bot > > results i thought it is ok. I don't know if i can do it conditionally similar > to > > what i am doing in RunEndedCallbackIfNeeded(). What do you suggest? > > If there's just a few tests which trigger this OnDuration change you should > explicitly add it to those tests. If there's a bunch (> 5) you can consider > switching to a NiceMock; though we'll want to hunt down what the real issue is > here. will check it. https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is On 2014/12/04 02:04:41, DaleCurtis wrote: > Same comment as PipelineIntegrationTest. Sorry, i couldn't get what you mean by.
https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... File media/base/pipeline_unittest.cc (right): https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... media/base/pipeline_unittest.cc:622: // There are cases where duration is reported wrong initially, so there is On 2014/12/04 03:16:46, Srirama wrote: > On 2014/12/04 02:04:41, DaleCurtis wrote: > > Same comment as PipelineIntegrationTest. > > Sorry, i couldn't get what you mean by. I meant don't add an EXPECT_CALL that won't always be fulfilled.
On 2014/12/04 19:10:44, DaleCurtis wrote: > https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... > File media/base/pipeline_unittest.cc (right): > > https://codereview.chromium.org/740663002/diff/440001/media/base/pipeline_uni... > media/base/pipeline_unittest.cc:622: // There are cases where duration is > reported wrong initially, so there is > On 2014/12/04 03:16:46, Srirama wrote: > > On 2014/12/04 02:04:41, DaleCurtis wrote: > > > Same comment as PipelineIntegrationTest. > > > > Sorry, i couldn't get what you mean by. > > I meant don't add an EXPECT_CALL that won't always be fulfilled. I ran the media unittests with the help of logs and there are only two tests which trigger this 2nd onduration change event. 1) PipelineTest.EndedCallback - So i have added EXPECT_CALL in TEST_F(PipelineTest, EndedCallback). 2) PipelineIntegrationTest.BasicPlaybackPositiveStartTime - here there is no check for onduration change event so it is already passing. Would you please let me know if anything else i need to check.
Ah, thanks for the clarity, sorry I misread the latest patchset. lgtm, thanks for your patience.
On 2014/12/09 05:31:00, DaleCurtis wrote: > Ah, thanks for the clarity, sorry I misread the latest patchset. lgtm, thanks > for your patience. No problem at all, thanks to you and philip for your guidance and patient reviews.
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/740663002/440001
Message was sent while issue was closed.
Committed patchset #10 (id:440001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/36ab2684f2422d6cb49c2462333816fb9857cfe6 Cr-Commit-Position: refs/heads/master@{#307857}
Message was sent while issue was closed.
On 2014/12/11 04:20:59, I haz the power (commit-bot) wrote: > Patchset 10 (id:??) landed as > https://crrev.com/36ab2684f2422d6cb49c2462333816fb9857cfe6 > Cr-Commit-Position: refs/heads/master@{#307857} Hi, I think we need to revisit this. Why did we end up invoking SetDuration in Pipeline::RunEndedCallbackIfNeeded? There is already (temporary) logic in WebMediaPlayerImpl::currentTime to return duration() if WebMediaPlayerImpl::ended_ is true. I think that should be enough to end playback properly in all cases. And the fact that we call Pipeline::SetDuration when ending playback causes problems for ending MSE playback, which causes some Chromecast (internal) unit test failures (b/19952396). I've been investigating this today, and here is what I think happens: 1. Pipeline::RunEndedCallbackIfNeeded is invoked to signal end of stream. It sets the new pipeline duration (current media_time=122.993, close to EOS, but not quite, actual EOS is at 123.252): 04-06 18:17:32.474 I/cast_shell( 3402): [24:32:INFO:pipeline.cc(671)] RunEndedCallbackIfNeeded media_time=122.993 dur=123.252 04-06 18:17:32.474 I/cast_shell( 3402): [24:32:INFO:pipeline.cc(295)] SetDuration 122.993 2. Pipeline::SetDuration triggers HTMLMediaElement::mediaPlayerDurationChanged 04-06 18:17:32.474 I/cast_shell( 3402): HTMLMediaElement::mediaPlayerDurationChanged(0x3fab4000) 3. But now take a closer look at HTMLMediaElement::duration, if we are going through WebMediaPlayer code path, then it'll go and ask duration from the media::Pipeline object (through WebMediaPlayer::duration) and everything is ok. But if we are doing MSE playback, HTMLMediaSource::duration will obtain actual duration from the ChunkDemuxer (via HTMLMediaSource::duration invoked at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). ChunkDemuxer still has the actual duration, not the one set in step #1: 04-06 18:17:32.474 I/cast_shell( 3402): HTMLMediaElement::durationChanged(0x3fab4000, 123.251519, 0) As a result HTMLMediaElement fails to recognise EOS and doesn't fire the "ended" event: 04-06 18:17:32.474 I/cast_shell( 3402): HTMLMediaElement::mediaPlayerTimeChanged(0x3fab4000) now=122.992671 dur=123.251519 04-06 18:17:32.474 I/cast_shell( 3402): HTMLMediaElement::updatePlayState(0x3fab4000) - shouldBePlaying = true, isPlaying = true So coming back to my question: Why do we need to do Pipeline::SetDuration instead of relying on currentTime being the same as duration when EOS reached?
Message was sent while issue was closed.
Patchset #11 (id:460001) has been deleted
Message was sent while issue was closed.
I think ultimately we want to get away from this currentTime == duration ==> ended signal and instead prefer an explicit signal. There's a bug somewhere filed for this. Doing so would let us remove several of these hacks. I see your point about MediaSource missing the necessary hack. I don't think Srirama is working on this anymore, it ultimately ended up causing some issues and was reverted, so it's tricky to get right. I'd love for someone to own this and clean everything up, but understand that the most practical approach might be to extend the hack to media source right now.
Message was sent while issue was closed.
On 2015/04/07 17:14:54, DaleCurtis wrote: > I think ultimately we want to get away from this currentTime == duration ==> > ended signal and instead prefer an explicit signal. There's a bug somewhere > filed for this. Doing so would let us remove several of these hacks. > > I see your point about MediaSource missing the necessary hack. I don't think > Srirama is working on this anymore, it ultimately ended up causing some issues > and was reverted, so it's tricky to get right. > > I'd love for someone to own this and clean everything up, but understand that > the most practical approach might be to extend the hack to media source right > now. Yeah, this is messy. FYI: I've opened bug crbug.com/475244 and I'm planning to partially revert this CL for now to fix the MSE playback regression. I think the proper way to fix this was described by scherkus@ here - https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed... I'm planning to try that approach after https://codereview.chromium.org/1055503002/ lands.
Message was sent while issue was closed.
This CL has already been reverted I thought?
Message was sent while issue was closed.
On 2015/04/08 22:01:52, DaleCurtis wrote: > This CL has already been reverted I thought? Nope, I still see SetDuration being invoked from Pipeline::RunEndedCallbackIfNeeded in top-of-tree Chromium. Do you know who might have tried to revert this? I can't find any relevant context in git/bugs.
Message was sent while issue was closed.
The bug in the description says this is reverted?
Message was sent while issue was closed.
On 2015/04/08 22:35:32, DaleCurtis wrote: > The bug in the description says this is reverted? What bug are you referring to? crbug.com/266631? There's a bunch of changes there, apparently yes, some CLs related to this were reverted. But I'm specifically concerned about this piece of code that calls Pipeline::SetDuration from Pipeline::RunEndedCallbackIfNeeded: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/pipelin... This is the thing that's causing problems for ending MSE playback and this is the thing that I'm reverting with my CL https://codereview.chromium.org/1069253004/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
