Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
456343002:
Relanding 'Always notify the MediaPlayer of any seek' patch (Closed)
|
Created:
6 years, 4 months ago by Srirama Modified:
5 years, 11 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 Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRelanding 'Always notify the MediaPlayer of any seek' patch
This change removes short circuit logic for seeking to the current position.
There are certain situations, like seeking to a truncated duration, where the
HTMLMediaElement is not in the best position to determine whether a call to
the MediaPlayer is necessary or not. This change allows the
MediaPlayer/WebMediaPlayer implementation to determine what the best course
of action should be. This also makes the seeking behavior more spec
compliant because even a seek to the current position should have the
seeking attribute be true until a stable state occurs. This is guaranteed
when calling into the MediaPlayer and allows all seeks to be treated the
same at the HTMLMediaElement level.
BUG=266631
TEST=LayoutTests/media/video-seek-to-current-position.html
Original CL: https://codereview.chromium.org/431903003/
Depends on the chromium patch https://codereview.chromium.org/685993002/
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188150
Patch Set 1 #Patch Set 2 : Updated test case #Patch Set 3 : Fixed test failure #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Fixed test failure #
Total comments: 1
Patch Set 6 : Fixed controls-restrained-media-controller.html failure #Patch Set 7 : Fixed LayoutTests/media/video-double-seek-currentTime.html failure #
Total comments: 1
Patch Set 8 : Fixed test failures #
Total comments: 1
Messages
Total messages: 50 (4 generated)
The regression caused previously (399523) was fixed by Dale Curtis (https://codereview.chromium.org/440803002) So i just uploaded the original patch without any modifications. PTAL
On 2014/08/11 11:37:09, Srirama wrote: > The regression caused previously (399523) was fixed by Dale Curtis > (https://codereview.chromium.org/440803002) > So i just uploaded the original patch without any modifications. > PTAL Why does Dale's patch fix the problem? I don't understand why changing seeking behavior would be affected by a fix to splicing behavior. How is mediasource-duration.html actually creating splices? Why does forcing a seek cause the old splicing code old splicing code to fail?
On 2014/08/11 17:18:31, acolwell wrote: > On 2014/08/11 11:37:09, Srirama wrote: > > The regression caused previously (399523) was fixed by Dale Curtis > > (https://codereview.chromium.org/440803002) > > So i just uploaded the original patch without any modifications. > > PTAL > > Why does Dale's patch fix the problem? I don't understand why changing seeking > behavior would be affected by a fix to splicing behavior. How is > mediasource-duration.html actually creating splices? Why does forcing a seek > cause the old splicing code old splicing code to fail? I don't know exactly what is happening at chromium side, but what i understood at a high level from the mediasource-duration.html test case and from the problem described in http://crbug.com/400442 is, there is a problem with MSE when the timeoffsets were beyond duration. Similar behaviour is getting simulated in the current test case because there is seek getting called with a time of 'X' and then the duration is forcefully being truncated to X/2. I am guessing that in these cases seek is being called to the current time again. In our patch we have removed bypassing these seek events and removed the check for pending seek event as well which may be simulating the behaviour described in http://crbug.com/400442 I will debug more with the help of logs tomorrow if required.
On 2014/08/11 18:31:38, Srirama wrote: > On 2014/08/11 17:18:31, acolwell wrote: > > On 2014/08/11 11:37:09, Srirama wrote: > > > The regression caused previously (399523) was fixed by Dale Curtis > > > (https://codereview.chromium.org/440803002) > > > So i just uploaded the original patch without any modifications. > > > PTAL > > > > Why does Dale's patch fix the problem? I don't understand why changing seeking > > behavior would be affected by a fix to splicing behavior. How is > > mediasource-duration.html actually creating splices? Why does forcing a seek > > cause the old splicing code old splicing code to fail? > > I don't know exactly what is happening at chromium side, but what i understood > at a high level from the mediasource-duration.html test case and from the > problem described in http://crbug.com/400442 is, > there is a problem with MSE when the timeoffsets were beyond duration. Similar > behaviour is getting simulated in the current test case because there is seek > getting called with a time of 'X' and then the duration is forcefully being > truncated to X/2. I am guessing that in these cases seek is being called to the > current time again. In our patch we have removed bypassing these seek events and > removed the check for pending seek event as well which may be simulating the > behaviour described in http://crbug.com/400442 > I will debug more with the help of logs tomorrow if required. I don't understand how these 2 issues are related. The duration that is being mentioned in that bug is the duration of individual compressed frames and not the duration of the whole presentation. I would not expect mediasource-duration.html to generate the same situation as bug 400442. Please make sure you fully understand and can explain exactly why the change in seek behavior resulted in us running into the bad splicing behavior and why Dale's fix is actually the right fix for the seeking issue as well. My worry is that there is still an underlying issue that we may be missing and I'm not convinced you understand what the original issue was or why Dale's change appears to have fixed it.
+1, I should have clarified that I was looking for insight into why your patch was hitting that issue. I wonder if the seek happens in the middle of a splice frame or something similar. You can see where splice frames are generated here: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... Applying this patch should add lots of relevant logs: http://pastebin.com/sUb5tpau
On 2014/08/11 20:17:29, DaleCurtis wrote: > +1, I should have clarified that I was looking for insight into why your patch > was hitting that issue. I wonder if the seek happens in the middle of a splice > frame or something similar. > > You can see where splice frames are generated here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... > > Applying this patch should add lots of relevant logs: > http://pastebin.com/sUb5tpau @acolwell, @DaleCurtis: thanks for the clarifications, i will look into it and understand why exactly the change in seek behavior is causing the issue.
On 2014/08/12 05:43:55, Srirama wrote: > On 2014/08/11 20:17:29, DaleCurtis wrote: > > +1, I should have clarified that I was looking for insight into why your patch > > was hitting that issue. I wonder if the seek happens in the middle of a splice > > frame or something similar. > > > > You can see where splice frames are generated here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... > > > > Applying this patch should add lots of relevant logs: > > http://pastebin.com/sUb5tpau > > @acolwell, @DaleCurtis: thanks for the clarifications, i will look into it and > understand why exactly the change in seek behavior is causing the issue. I was not able to access http://pastebin.com/sUb5tpau in my office. So i have checked by adding some logs in audio_splicer. For success case (when the test is run once) it is returning from if (splice_timestamp_ == kNoTimestamp()) condition in AudioSplicer::AddInput() where as when i run the test more than once it is going further as the splice_timestamp_ is being set. So on my further investigation found that it is being set from AudioRendererImpl::OnNewSpliceBuffer(). I will check further how this is getting set. Hopefully i will be able to find the reason with the help of logs in http://pastebin.com/sUb5tpau tomorrow.
Thanks for investigating. I've emailed you the patch in case you still can't get it from Pastebin.
On 2014/08/12 22:32:10, DaleCurtis wrote: > Thanks for investigating. I've emailed you the patch in case you still can't get > it from Pastebin. I have applied the logs and simulated the issue. But i think from the logs it is not clear why slice frames are generated. I need to check further i guess why slice frames are coming into picture in this test case. Please have a quick look at the logs if you can make out something. I will be out of office for one week, will keep checking if possible or will continue once i am back, sorry for that. Please have a quick look at the logs updated in http://code.google.com/p/chromium/issues/detail?id=399523
What is the status of the investigation around this patch? I'd really like to see this resolved as soon as possible.
On 2014/09/08 19:13:38, acolwell wrote: > What is the status of the investigation around this patch? I'd really like to > see this resolved as soon as possible. I have updated latest logs based on the minimized test case. I am not able to get exactly what is the issue there but from the success and failure logs i just see the difference of the time the seek is being called. Please have a quick look and tell me where can i debug further.
acolwell@chromium.org changed reviewers: - acolwell@chromium.org
On 2014/09/09 14:03:52, Srirama wrote:
> On 2014/09/08 19:13:38, acolwell wrote:
> > What is the status of the investigation around this patch? I'd really like
to
> > see this resolved as soon as possible.
>
> I have updated latest logs based on the minimized test case. I am not able to
> get exactly what is the issue there but from the success and failure logs i
just
> see the difference of the time the seek is being called. Please have a quick
> look and tell me where can i debug further.
As discussed i have tried moving the noseekrequired logic to
webmediaplayer_impl. I just modified as below. Please correct me if i am wrong.
With these changes the tests are timing out as we are not sending the seeking
and seeked events.
Orig:
if (paused_) {
paused_time_ = seek_time;
Modified:
if (paused_) {
if (paused_time_ != seek_time)
paused_time_ = seek_time;
else
return;
}
On 2014/10/28 14:26:54, Srirama wrote:
> On 2014/09/09 14:03:52, Srirama wrote:
> > On 2014/09/08 19:13:38, acolwell wrote:
> > > What is the status of the investigation around this patch? I'd really like
> to
> > > see this resolved as soon as possible.
> >
> > I have updated latest logs based on the minimized test case. I am not able
to
> > get exactly what is the issue there but from the success and failure logs i
> just
> > see the difference of the time the seek is being called. Please have a quick
> > look and tell me where can i debug further.
>
> As discussed i have tried moving the noseekrequired logic to
> webmediaplayer_impl. I just modified as below. Please correct me if i am
wrong.
> With these changes the tests are timing out as we are not sending the seeking
> and seeked events.
>
> Orig:
> if (paused_) {
> paused_time_ = seek_time;
>
> Modified:
> if (paused_) {
> if (paused_time_ != seek_time)
> paused_time_ = seek_time;
> else
> return;
> }
To get those events shouldn't it be on the media thread? I think this is what
acolwell has been insisting on so that we get proper seeking and seeked events
for all seek operations (including noop seeks)
Please upload your patch for webmediaplayer_impl as a separate review so we can see what you've done.
Rebased and uploaded the patch. Updated the test case LayoutTests/media/video-seek-to-current-position.html to include timeupdate event. This should land after the chromium patch https://codereview.chromium.org/685993002/
On 2014/10/30 11:10:48, Srirama wrote: > Rebased and uploaded the patch. Updated the test case > LayoutTests/media/video-seek-to-current-position.html to include timeupdate > event. > This should land after the chromium patch > https://codereview.chromium.org/685993002/ Test failures media/media-controller-playbackrate.html - Is passing with chromium patch media/controls-timeline-with-controller.html - Looks like recent changes have modified the behaviour, i am checking it media/media-controller-effective-playback-rate.html - Is passing with chromium patch
Patchset #3 (id:40001) has been deleted
Updated the test case media/controls-timeline-with-controller.html to fix the failure. Added seeked event to make sure there is a stable state before we query the m_seeking attribute. Other two failures will be fixed when the chromium patch lands (https://codereview.chromium.org/685993002/).
The Chromium patch has landed now, can you run the bots again to verify no failures?
LGTM It seems fairly likely that some content is going to be affected by this change, but I think it's the right thing to do and aligning with the spec is the only way to find out if it's Web compatible. https://codereview.chromium.org/456343002/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-seek-to-current-position.html (right): https://codereview.chromium.org/456343002/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-seek-to-current-position.html:8: <script src="video-test.js"></script> Please try to use testharness.js for new tests, so that they can more easily be shared with other browsers via web-platform-tests. No need to rewrite this one, though.
On 2014/11/14 12:21:05, philipj wrote: > LGTM > > It seems fairly likely that some content is going to be affected by this change, > but I think it's the right thing to do and aligning with the spec is the only > way to find out if it's Web compatible. > > https://codereview.chromium.org/456343002/diff/60001/LayoutTests/media/video-... > File LayoutTests/media/video-seek-to-current-position.html (right): > > https://codereview.chromium.org/456343002/diff/60001/LayoutTests/media/video-... > LayoutTests/media/video-seek-to-current-position.html:8: <script > src="video-test.js"></script> > Please try to use testharness.js for new tests, so that they can more easily be > shared with other browsers via web-platform-tests. No need to rewrite this one, > though. Actually the chromium change has been reverted yesterday https://codereview.chromium.org/726523003/ I was looking into that. I was checking the bots here but couldn't figure out which one to run for Linux ChromiumOS tests.
Bah, landing this change has turned out to be hard :)
Test Failure: media/controls-restrained-media-controller.html Image diff shows the frame is not painted, only controls are shown. Please find the details of my findings for the failure. There is an internal seek when pipeline starts (seek to 0). There is a seek from HTMLMediaElement (seek to 0 again) which will be forwarded because of our patch. By the time seek goes to webmediaplayer, the previous seek is in progress. So it is queued and OnPipelineSeeked invokes the pending seek. The only difference before and after the patch is onPipelineSeeked() is not executing completely because of pending seek. When i add dummy OnPipelineSeeked() event to my chromium patch the test case is passing. All it is doing is just call pipeline_->GetMediaTime(). I am not sure how this is making sure frame is painted (may be some internal refresh). Shall i add OnPipelineSeeked() and proceed?
That's a Chromium-side question I can't answer well. Dale?
Probably paused_time_ has drifted slightly from GetMediaTime somehow, so calling that method causes the paused_time_ to be updated again.
Test is flaky, need to investigate more.
@philip, what do you think of patchset5? I did a bit of analysis at chromium side with logs in webmediaplayer but the logs are same for both fail and success case. So i thought timing may be the reason for the test flakiness. I tried with different events (seeked, loadeddata instead of canplay) but still it is flaky. So i tried with settimeout to give enough time for the test to get correct frame. Please let me know if this is good enough to land this change.
On 2014/12/15 12:48:29, Srirama wrote: > @philip, what do you think of patchset5? > I did a bit of analysis at chromium side with logs in webmediaplayer but the > logs are same for both fail and success case. So i thought timing may be the > reason for the test flakiness. I tried with different events (seeked, loadeddata > instead of canplay) but still it is flaky. > So i tried with settimeout to give enough time for the test to get correct > frame. Please let me know if this is good enough to land this change. The win bot failure is unrelated.
It's too bad we're spending time on MediaController again, but I'd like to understand what's going on here first... https://codereview.chromium.org/456343002/diff/100001/LayoutTests/media/contr... File LayoutTests/media/controls-restrained-media-controller.html (right): https://codereview.chromium.org/456343002/diff/100001/LayoutTests/media/contr... LayoutTests/media/controls-restrained-media-controller.html:16: window.setTimeout(function() { Having a timeout is a recipe for future flakiness. From the failure it looks like we're not guaranteed to have decoded and painted the first frame in the canplay event, which seems very odd. It also seems to have failed on all bots, so is it actually flaky? The test doesn't even explicitly set currentTime, so how is it related to the change this CL makes?
On 2014/12/15 14:52:38, philipj wrote: > It's too bad we're spending time on MediaController again, but I'd like to > understand what's going on here first... > > https://codereview.chromium.org/456343002/diff/100001/LayoutTests/media/contr... > File LayoutTests/media/controls-restrained-media-controller.html (right): > > https://codereview.chromium.org/456343002/diff/100001/LayoutTests/media/contr... > LayoutTests/media/controls-restrained-media-controller.html:16: > window.setTimeout(function() { > Having a timeout is a recipe for future flakiness. From the failure it looks > like we're not guaranteed to have decoded and painted the first frame in the > canplay event, which seems very odd. It also seems to have failed on all bots, > so is it actually flaky? The test doesn't even explicitly set currentTime, so > how is it related to the change this CL makes? Sorry, flaky i mean it is passing 3 out of 10 times in my linux build. And as i explained in my previous comment(#24), there is a seek to currentTime, currentTime being zero here and the media is in pause state, so the chromium patch(https://codereview.chromium.org/740663002/) which i have done earlier is causing this issue.
If there are issues with your previous patch, please revert it until you've figured out the root cause.
From comment #24 it sounds like the problem is a new seek before the previous one has finished. Per https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-seek this should end up seeking to the new time, but if that new target time is the same as the previous target I think ignoring the new seek request would be indistinguishable from canceling the first request and performing the second one. The Chromium side could handle this, but you could also try "if (m_seeking && time == m_lastSeekTime) return" in HTMLMediaElement::seek() right after "time = seekableRanges->nearest(time, now)"
On 2014/12/15 19:51:53, philipj wrote: > From comment #24 it sounds like the problem is a new seek before the previous > one has finished. Per > https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-seek this > should end up seeking to the new time, but if that new target time is the same > as the previous target I think ignoring the new seek request would be > indistinguishable from canceling the first request and performing the second > one. The Chromium side could handle this, but you could also try "if (m_seeking > && time == m_lastSeekTime) return" in HTMLMediaElement::seek() right after "time > = seekableRanges->nearest(time, now)" Hmm, that will be a good fix if it works out but i doubt m_seeing will be true. The first seek is initiated by pipeline internally, so HTMLMediaElement may not be aware of that. Anyway let me confirm that.
On 2014/12/15 18:52:48, DaleCurtis wrote: > If there are issues with your previous patch, please revert it until you've > figured out the root cause. The initial chromium patch, in which i have added noseekrequired logic along with this blink patch is creating problem for this test case, rest is fine. And that patch has some of the fixes for duration issues as well. So instead of reverting that it will be better to fix this failure IMO.
On 2014/12/16 05:36:46, Srirama wrote: > On 2014/12/15 18:52:48, DaleCurtis wrote: > > If there are issues with your previous patch, please revert it until you've > > figured out the root cause. > > The initial chromium patch, in which i have added noseekrequired logic along > with this blink patch is creating problem for this test case, rest is fine. And > that patch has some of the fixes for duration issues as well. So instead of > reverting that it will be better to fix this failure IMO. m_seeing is set to true at the beginning of the function, so checking for m_seeking is of no value and i have verified if there is a seek pending by checking the value just before setting and it is false in this case, because the seek is an internal pipeline seek.
Sounds like a fix on the Chromiums side is needed, then.
On 2014/12/16 09:54:27, philipj wrote: > Sounds like a fix on the Chromiums side is needed, then. OK, i will work on that.
Patchset #6 (id:120001) has been deleted
PTAL at PatchSet7. Modified as per your suggestion with the help of webMediaPlayer()->seeking(). Updated LayoutTests/media/video-double-seek-currentTime.html as the behaviour is changed a bit.
Ping
https://codereview.chromium.org/456343002/diff/160001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/456343002/diff/160001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:2029: if (webMediaPlayer()->seeking() && time == m_lastSeekTime) I don't know, it seems odd that WebMediaPlayer::seeking() is even exposed, the only other place where it's used also seems like a bit of a workaround at the wrong layer. If you tried to explain this in a comment, I think it would become clear that this depends on WebMediaPlayer-internal details: "If the player is seeking internally without HTMLMediaElement knowing about it, do nothing and wait for it to notify us that a seek has completed even though we didn't ask for it" Was there any trouble in attempting a Chromium-side fix?
On 2014/12/19 15:06:03, philipj wrote: > https://codereview.chromium.org/456343002/diff/160001/Source/core/html/HTMLMe... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/456343002/diff/160001/Source/core/html/HTMLMe... > Source/core/html/HTMLMediaElement.cpp:2029: if (webMediaPlayer()->seeking() && > time == m_lastSeekTime) > I don't know, it seems odd that WebMediaPlayer::seeking() is even exposed, the > only other place where it's used also seems like a bit of a workaround at the > wrong layer. > > If you tried to explain this in a comment, I think it would become clear that > this depends on WebMediaPlayer-internal details: "If the player is seeking > internally without HTMLMediaElement knowing about it, do nothing and wait for it > to notify us that a seek has completed even though we didn't ask for it" > > Was there any trouble in attempting a Chromium-side fix? As far as i have tried, either controls-restrained-media-controller.html fails or video-seek-to-current-position.html fails. During that investigation only i found that webmediaplayer()->seeking() is true even for internal seek. So i am able to make a fix at HTMLMediaElement with that. If you feel it is better to fix at chromium side, i will try more on that.
It sounds like you're not exactly sure about the root cause yet. If the problem is in some way related to internal seeks on the Chromium side, then yes, fixing it on that side seems like the best course of action.
On 2014/12/19 22:38:42, philipj wrote: > It sounds like you're not exactly sure about the root cause yet. If the problem > is in some way related to internal seeks on the Chromium side, then yes, fixing > it on that side seems like the best course of action. @philipj: Chromium change https://codereview.chromium.org/787373008/ has landed. Shall i submit this one now?
The CQ bit was checked by philipj@opera.com
lgtm LGTM, I hope it sticks this time! https://codereview.chromium.org/456343002/diff/180001/LayoutTests/media/media... File LayoutTests/media/media-controller-playbackrate.html (left): https://codereview.chromium.org/456343002/diff/180001/LayoutTests/media/media... LayoutTests/media/media-controller-playbackrate.html:7: var start = function() { Both styles are OK by http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-B... so you don't need to change these unless they're inconsistent within a test. No need to revert this, though.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/456343002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188150
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:180001) has been created in https://codereview.chromium.org/848853003/ by srirama.m@samsung.com. The reason for reverting is: Android perf bots failure in media.android.tough_video_cases (timeout) https://code.google.com/p/chromium/issues/detail?id=448092. |
