Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
530993002:
WebMediaPlayerImpl should notify ended event
|
Created:
6 years, 3 months ago by amogh.bihani Modified:
6 years, 3 months ago CC:
blink-reviews, jamesr, blink-reviews-html_chromium.org, Rik, pdr., gasubic, fs, eric.carlson_apple.com, jbroman, danakj, feature-media-reviews_chromium.org, dglazkov+blink, nessy, krit, Stephen Chennney, abarth-chromium, vcarbune.chromium, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionWebMediaPlayerImpl should notify ended event
HTMLMediaElement determines ended event by comparing two doubles. Sometimes
it happens that the current time is not exactly equal to the duration.
Because of this the event is not fired. This patch relays the ended event
notification from WebMediaPlayerImpl to HTMLMediaElement. This will avoid
comparision between two doubles.
BUG=409280, 349543
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressing comments #
Total comments: 4
Patch Set 3 : Shortening comments #Patch Set 4 : removing use of m_sentEndEvent #
Total comments: 8
Patch Set 5 : Rebase #
Messages
Total messages: 33 (2 generated)
amogh.bihani@samsung.com changed reviewers: + scherkus@chromium.org
PTAL. Please note that this will go in two parts. The duplicate part will be removed once https://codereview.chromium.org/533543004/ lands.
https://codereview.chromium.org/530993002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3103: // If the media element has a loop attribute specified and does not have a current media controller, is it possible to refactor your new code to call mediaPlayerEnded() when the time comparison is met? https://codereview.chromium.org/530993002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3134: void HTMLMediaElement::mediaPlayerEnded() one thing that's tricky is that with this new code path it's possible that currentTime will be less than duration (e.g., container's stated duration was larger than what we actually decoded), which isn't what the spec states I believe as an interim step we may need to change currentTime() to check for m_sentEndEvent and return duration()
Thanks for the review :) I have made that changes. https://codereview.chromium.org/530993002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3103: // If the media element has a loop attribute specified and does not have a current media controller, On 2014/09/02 20:23:54, scherkus wrote: > is it possible to refactor your new code to call mediaPlayerEnded() when the > time comparison is met? Done. https://codereview.chromium.org/530993002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3134: void HTMLMediaElement::mediaPlayerEnded() On 2014/09/02 20:23:54, scherkus wrote: > one thing that's tricky is that with this new code path it's possible that > currentTime will be less than duration (e.g., container's stated duration was > larger than what we actually decoded), which isn't what the spec states > > I believe as an interim step we may need to change currentTime() to check for > m_sentEndEvent and return duration() Done.
https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:2112: Sorry, I'll remove this in next PS.
few nits https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:2112: On 2014/09/03 05:19:09, amogh.bihani wrote: > Sorry, I'll remove this in next PS. thanks :) https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3104: // TODO(amogh.bihani): Remove this once chromium change lands.---------------- can you avoid using the "-----" comments? we don't typically use a lot of ASCII art (the TODO itself is fine, but I'd also link to your existing Chromium-side code review) https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3110: // --------------------------------------------------------------------------- remove this whole line
On 2014/09/03 17:16:45, scherkus wrote: > few nits > > https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... > Source/core/html/HTMLMediaElement.cpp:2112: > On 2014/09/03 05:19:09, amogh.bihani wrote: > > Sorry, I'll remove this in next PS. > > thanks :) > > https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... > Source/core/html/HTMLMediaElement.cpp:3104: // TODO(amogh.bihani): Remove this > once chromium change lands.---------------- > can you avoid using the "-----" comments? we don't typically use a lot of ASCII > art (the TODO itself is fine, but I'd also link to your existing Chromium-side > code review) > > https://codereview.chromium.org/530993002/diff/20001/Source/core/html/HTMLMed... > Source/core/html/HTMLMediaElement.cpp:3110: // > --------------------------------------------------------------------------- > remove this whole line Thanks. I have made all the changes.
Is the new model that the WebMediaPlayer doesn't have to care about reaching currentTime==duration, but instead can invoke mediaEnded() and have Blink clamp to duration until the next seek or load? That works, but should there any limit to how wrong it can be? It seems to me that there are two situations here: 1. The duration reported in the headers was wrong, and the final currentTime is as far as there is any data. 2. The media framework for some reason stops short of duration, but duration is correct. We're only dealing with (2) now, is there any testing of (1) using media files with an inaccurate duration that might now do the wrong thing?
On 2014/09/04 14:53:16, philipj wrote: > Is the new model that the WebMediaPlayer doesn't have to care about reaching > currentTime==duration, but instead can invoke mediaEnded() and have Blink clamp > to duration until the next seek or load? Correct. > That works, but should there any limit to how wrong it can be? > > It seems to me that there are two situations here: > > 1. The duration reported in the headers was wrong, and the final currentTime is > as far as there is any data. > > 2. The media framework for some reason stops short of duration, but duration is > correct. > > We're only dealing with (2) now, is there any testing of (1) using media files > with an inaccurate duration that might now do the wrong thing? Today we're handling (1) and (2) the same regardless of whether we're off by milliseconds or minutes. This set of CLs gets us closer to having blink do the clamping rather than having the various WebMediaPlayer implementations do it. I feel that's OK for now. I've also been thinking about future improvements where we can fix up the duration value once we know the "true" duration and fire a durationchange event, but I haven't really fully thought that one through yet TBH.
amogh.bihani@samsung.com changed reviewers: + philipj@opera.com
On 2014/09/04 21:36:21, scherkus wrote: > On 2014/09/04 14:53:16, philipj wrote: > > Is the new model that the WebMediaPlayer doesn't have to care about reaching > > currentTime==duration, but instead can invoke mediaEnded() and have Blink > clamp > > to duration until the next seek or load? > > Correct. > > > That works, but should there any limit to how wrong it can be? > > > > It seems to me that there are two situations here: > > > > 1. The duration reported in the headers was wrong, and the final currentTime > is > > as far as there is any data. > > > > 2. The media framework for some reason stops short of duration, but duration > is > > correct. > > > > We're only dealing with (2) now, is there any testing of (1) using media files > > with an inaccurate duration that might now do the wrong thing? > > Today we're handling (1) and (2) the same regardless of whether we're off by > milliseconds or minutes. This set of CLs gets us closer to having blink do the > clamping rather than having the various WebMediaPlayer implementations do it. I > feel that's OK for now. > > I've also been thinking about future improvements where we can fix up the > duration value once we know the "true" duration and fire a durationchange event, > but I haven't really fully thought that one through yet TBH. So whats done here is fine or something needs to be modified? And a small doubt, in android we do OnTimeUpdate(duration_) in OnPlaybackComplete() to clamp to the duration, but why don't we do the same in webmediaplayer_impl in OnPipelineEnded()?
On 2014/09/04 21:36:21, scherkus wrote: > On 2014/09/04 14:53:16, philipj wrote: > > Is the new model that the WebMediaPlayer doesn't have to care about reaching > > currentTime==duration, but instead can invoke mediaEnded() and have Blink > clamp > > to duration until the next seek or load? > > Correct. > > > That works, but should there any limit to how wrong it can be? > > > > It seems to me that there are two situations here: > > > > 1. The duration reported in the headers was wrong, and the final currentTime > is > > as far as there is any data. > > > > 2. The media framework for some reason stops short of duration, but duration > is > > correct. > > > > We're only dealing with (2) now, is there any testing of (1) using media files > > with an inaccurate duration that might now do the wrong thing? > > Today we're handling (1) and (2) the same regardless of whether we're off by > milliseconds or minutes. This set of CLs gets us closer to having blink do the > clamping rather than having the various WebMediaPlayer implementations do it. I > feel that's OK for now. > > I've also been thinking about future improvements where we can fix up the > duration value once we know the "true" duration and fire a durationchange event, > but I haven't really fully thought that one through yet TBH. When mediaEnded() is called when currentTime != duration, it seems Blink isn't really in a position to judge which of the two is wrong. I think the end state to strive for is that the backend updates currentTime or duration before calling mediaEnded(). In particular, I'd really like to get rid of m_sentEndEvent, but this CL expands it use. How about letting mediaPlayerEnded() ensure that currentTime == duration, first by just making it so, but eventually by asserting it? Right now, it looks like the least messy thing would be to update the duration by calling refreshCachedTime() and durationChanged(currentTime(), false). With a non-buggy media backend, that seems to be the reasonable outcome, since duration metadata can easily be wrong, but it's weird if playback stops short (or beyond) a correct duration. WDYT?
On 2014/09/08 08:11:53, philipj wrote: > On 2014/09/04 21:36:21, scherkus wrote: > > On 2014/09/04 14:53:16, philipj wrote: > > > Is the new model that the WebMediaPlayer doesn't have to care about reaching > > > currentTime==duration, but instead can invoke mediaEnded() and have Blink > > clamp > > > to duration until the next seek or load? > > > > Correct. > > > > > That works, but should there any limit to how wrong it can be? > > > > > > It seems to me that there are two situations here: > > > > > > 1. The duration reported in the headers was wrong, and the final currentTime > > is > > > as far as there is any data. > > > > > > 2. The media framework for some reason stops short of duration, but duration > > is > > > correct. > > > > > > We're only dealing with (2) now, is there any testing of (1) using media > files > > > with an inaccurate duration that might now do the wrong thing? > > > > Today we're handling (1) and (2) the same regardless of whether we're off by > > milliseconds or minutes. This set of CLs gets us closer to having blink do the > > clamping rather than having the various WebMediaPlayer implementations do it. > I > > feel that's OK for now. > > > > I've also been thinking about future improvements where we can fix up the > > duration value once we know the "true" duration and fire a durationchange > event, > > but I haven't really fully thought that one through yet TBH. > > When mediaEnded() is called when currentTime != duration, it seems Blink isn't > really in a position to judge which of the two is wrong. I think the end state > to strive for is that the backend updates currentTime or duration before calling > mediaEnded(). That seems cool. Plus I would say that we should update duration as soon as currentTime is more than duration so that the media slider is painted properly[1]. Since there is no guard present to limit current position to rect.Width() we might paint more than that. But on the other hand it is a very corner case and requires a lot of computation for constant updation. Need suggestions on that. --------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > In particular, I'd really like to get rid of m_sentEndEvent, but this CL expands > it use. How about letting mediaPlayerEnded() ensure that currentTime == > duration, first by just making it so, but eventually by asserting it? Sure, then we shall reduce its use. :) > > Right now, it looks like the least messy thing would be to update the duration > by calling refreshCachedTime() and durationChanged(currentTime(), false). With a > non-buggy media backend, that seems to be the reasonable outcome, since duration > metadata can easily be wrong, but it's weird if playback stops short (or beyond) > a correct duration. > > WDYT?
On 2014/09/09 05:49:42, amogh.bihani wrote: > On 2014/09/08 08:11:53, philipj wrote: > > On 2014/09/04 21:36:21, scherkus wrote: > > > On 2014/09/04 14:53:16, philipj wrote: > > > > Is the new model that the WebMediaPlayer doesn't have to care about > reaching > > > > currentTime==duration, but instead can invoke mediaEnded() and have Blink > > > clamp > > > > to duration until the next seek or load? > > > > > > Correct. > > > > > > > That works, but should there any limit to how wrong it can be? > > > > > > > > It seems to me that there are two situations here: > > > > > > > > 1. The duration reported in the headers was wrong, and the final > currentTime > > > is > > > > as far as there is any data. > > > > > > > > 2. The media framework for some reason stops short of duration, but > duration > > > is > > > > correct. > > > > > > > > We're only dealing with (2) now, is there any testing of (1) using media > > files > > > > with an inaccurate duration that might now do the wrong thing? > > > > > > Today we're handling (1) and (2) the same regardless of whether we're off by > > > milliseconds or minutes. This set of CLs gets us closer to having blink do > the > > > clamping rather than having the various WebMediaPlayer implementations do > it. > > I > > > feel that's OK for now. > > > > > > I've also been thinking about future improvements where we can fix up the > > > duration value once we know the "true" duration and fire a durationchange > > event, > > > but I haven't really fully thought that one through yet TBH. > > > > When mediaEnded() is called when currentTime != duration, it seems Blink isn't > > really in a position to judge which of the two is wrong. I think the end state > > to strive for is that the backend updates currentTime or duration before > calling > > mediaEnded(). > > That seems cool. Plus I would say that we should update duration as soon as > currentTime is more than duration so that the media slider is painted > properly[1]. Since there is no guard present to limit current position to > rect.Width() we might paint more than that. But on the other hand it is a very > corner case and requires a lot of computation for constant updation. > Need suggestions on that. > > --------- > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > In particular, I'd really like to get rid of m_sentEndEvent, but this CL > expands > > it use. How about letting mediaPlayerEnded() ensure that currentTime == > > duration, first by just making it so, but eventually by asserting it? > > Sure, then we shall reduce its use. :) > > > > > Right now, it looks like the least messy thing would be to update the duration > > by calling refreshCachedTime() and durationChanged(currentTime(), false). With > a > > non-buggy media backend, that seems to be the reasonable outcome, since > duration > > metadata can easily be wrong, but it's weird if playback stops short (or > beyond) > > a correct duration. > > > > WDYT? Changes done. PTAL.
On 2014/09/09 05:49:42, amogh.bihani wrote: > That seems cool. Plus I would say that we should update duration as soon as > currentTime is more than duration so that the media slider is painted > properly[1]. Since there is no guard present to limit current position to > rect.Width() we might paint more than that. But on the other hand it is a very > corner case and requires a lot of computation for constant updation. > Need suggestions on that. Do we have WebMediaPlayer implementations where this can happen? I think it would be ideal if the WebMediaPlayers would promise that currentTime <= duration, but just as with this issue there may be something one could do in a transition period.
https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3080: if (!std::isnan(dur) && dur && now >= dur && directionOfPlayback() == Forward) This is also temporary, to be removed after codereview.chromium.org/533543004/ right? https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3088: WTF_LOG(Media, "HTMLMediaElement::mediaPlayerEnded"); There was a recent patch to add the this pointer to the logging, please do the same when rebasing this. https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3126: refreshCachedTime(); This looks correct, but is it related to the other changes or an independent fix? The several layers of times are a bit confusing at present :)
On 2014/09/10 12:56:03, philipj wrote: > On 2014/09/09 05:49:42, amogh.bihani wrote: > > > That seems cool. Plus I would say that we should update duration as soon as > > currentTime is more than duration so that the media slider is painted > > properly[1]. Since there is no guard present to limit current position to > > rect.Width() we might paint more than that. But on the other hand it is a very > > corner case and requires a lot of computation for constant updation. > > Need suggestions on that. > > Do we have WebMediaPlayer implementations where this can happen? I think it > would be ideal if the WebMediaPlayers would promise that currentTime <= > duration, but just as with this issue there may be something one could do in a > transition period. In webmediaplayer_android, we have OnTimeUpdate() which is constantly upadated with internal timer[1]. In webmediaplayer_impl, we do not have anything like that. I guess we can put a if check in currentTime(). It gets called pretty frequently. if currentTime > duration we can call durationChanged(). ------ [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3086: void HTMLMediaElement::mediaPlayerEnded() Do you intend to clamp duration to currentTime somewhere around here?
On 2014/09/10 13:14:45, amogh.bihani wrote: > On 2014/09/10 12:56:03, philipj wrote: > > On 2014/09/09 05:49:42, amogh.bihani wrote: > > > > > That seems cool. Plus I would say that we should update duration as soon as > > > currentTime is more than duration so that the media slider is painted > > > properly[1]. Since there is no guard present to limit current position to > > > rect.Width() we might paint more than that. But on the other hand it is a > very > > > corner case and requires a lot of computation for constant updation. > > > Need suggestions on that. > > > > Do we have WebMediaPlayer implementations where this can happen? I think it > > would be ideal if the WebMediaPlayers would promise that currentTime <= > > duration, but just as with this issue there may be something one could do in a > > transition period. > > In webmediaplayer_android, we have OnTimeUpdate() which is constantly upadated > with internal timer[1]. > In webmediaplayer_impl, we do not have anything like that. I guess we can put a > if check in currentTime(). It gets called pretty frequently. > if currentTime > duration we can call durationChanged(). > > ------ > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... OK. I'm in favor of fixing these problems in the WebMediaPlayers as far as possible, since on this side we're just guessing about which is correct of the reported currentTime and duration.
https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3080: if (!std::isnan(dur) && dur && now >= dur && directionOfPlayback() == Forward) On 2014/09/10 13:11:08, philipj wrote: > This is also temporary, to be removed after codereview.chromium.org/533543004/ > right? Yes. https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3086: void HTMLMediaElement::mediaPlayerEnded() On 2014/09/10 13:24:35, philipj wrote: > Do you intend to clamp duration to currentTime somewhere around here? I am planning to clamp it in webmediaplayer_impl side. https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3088: WTF_LOG(Media, "HTMLMediaElement::mediaPlayerEnded"); On 2014/09/10 13:11:08, philipj wrote: > There was a recent patch to add the this pointer to the logging, please do the > same when rebasing this. Acknowledged. https://codereview.chromium.org/530993002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3126: refreshCachedTime(); On 2014/09/10 13:11:08, philipj wrote: > This looks correct, but is it related to the other changes or an independent > fix? The several layers of times are a bit confusing at present :) It is related to the changes done in chromium side CL.
This now LGTM, but I'd like scherkus to take a final look, so that we aren't pulling in two directions. To be clear, where I think we should end up is: * WebMediaPlayer implementation guarantee that currentTime < duration during playback and currentTime == duration at playback end. * This is asserted in HTMLMediaElement. * The mediaEnded() callback is just a convenience, guaranteed to only be called once, so that HTMLMediaElement doesn't need to a flag to say if it has fired the ended event. (Although, if WebMediaPlayer also promises to not call mediaPlayerTimeChanged() twice, it also wouldn't be needed.)
On 2014/09/10 13:52:02, philipj wrote: > This now LGTM, but I'd like scherkus to take a final look, so that we aren't > pulling in two directions. To be clear, where I think we should end up is: > > * WebMediaPlayer implementation guarantee that currentTime < duration during > playback and currentTime == duration at playback end. This can be problematic especially in the MSE case where JS explicity sets the duration to a value < the currentTime. Ideally it would be nice if the WebMediaPlayer just reported what the current time is w/o the duration clamp so that the Blink code can actually detect that it needs to seek backwards in response to the duration change. Currently we have hacks for this. Just take a look at HTMLMediaElement::durationChanged() callers.
On 2014/09/10 17:44:05, acolwell wrote: > On 2014/09/10 13:52:02, philipj wrote: > > This now LGTM, but I'd like scherkus to take a final look, so that we aren't > > pulling in two directions. To be clear, where I think we should end up is: > > > > * WebMediaPlayer implementation guarantee that currentTime < duration during > > playback and currentTime == duration at playback end. > > This can be problematic especially in the MSE case where JS explicity sets the > duration to a value < the currentTime. Ideally it would be nice if the > WebMediaPlayer just reported what the current time is w/o the duration clamp so > that the Blink code can actually detect that it needs to seek backwards in > response to the duration change. Currently we have hacks for this. Just take a > look at HTMLMediaElement::durationChanged() callers. Does Blink get the updated duration value before WMPs do? If so, wouldn't the old duration be larger and currentTime still accurate?
On 2014/09/10 17:54:29, scherkus wrote: > On 2014/09/10 17:44:05, acolwell wrote: > > On 2014/09/10 13:52:02, philipj wrote: > > > This now LGTM, but I'd like scherkus to take a final look, so that we aren't > > > pulling in two directions. To be clear, where I think we should end up is: > > > > > > * WebMediaPlayer implementation guarantee that currentTime < duration > during > > > playback and currentTime == duration at playback end. > > > > This can be problematic especially in the MSE case where JS explicity sets the > > duration to a value < the currentTime. Ideally it would be nice if the > > WebMediaPlayer just reported what the current time is w/o the duration clamp > so > > that the Blink code can actually detect that it needs to seek backwards in > > response to the duration change. Currently we have hacks for this. Just take a > > look at HTMLMediaElement::durationChanged() callers. > > Does Blink get the updated duration value before WMPs do? If so, wouldn't the > old duration be larger and currentTime still accurate? Yes, which is why we need to have the brittle sequence of events in MediaSource::durationChangeAlgorithm() (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). Note on line 401 we need to cache that information before we notify the HTMLMediaElement of the duration change because once we tell the Chromium code about it, HTMLMediaElement can detect the seek condition.
On 2014/09/10 18:09:10, acolwell wrote: > On 2014/09/10 17:54:29, scherkus wrote: > > On 2014/09/10 17:44:05, acolwell wrote: > > > On 2014/09/10 13:52:02, philipj wrote: > > > > This now LGTM, but I'd like scherkus to take a final look, so that we > aren't > > > > pulling in two directions. To be clear, where I think we should end up is: > > > > > > > > * WebMediaPlayer implementation guarantee that currentTime < duration > > during > > > > playback and currentTime == duration at playback end. > > > > > > This can be problematic especially in the MSE case where JS explicity sets > the > > > duration to a value < the currentTime. Ideally it would be nice if the > > > WebMediaPlayer just reported what the current time is w/o the duration clamp > > so > > > that the Blink code can actually detect that it needs to seek backwards in > > > response to the duration change. Currently we have hacks for this. Just take > a > > > look at HTMLMediaElement::durationChanged() callers. > > > > Does Blink get the updated duration value before WMPs do? If so, wouldn't the > > old duration be larger and currentTime still accurate? > > Yes, which is why we need to have the brittle sequence of events in > MediaSource::durationChangeAlgorithm() > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > Note on line 401 we need to cache that information before we notify the > HTMLMediaElement of the duration change because once we tell the Chromium code > about it, HTMLMediaElement can detect the seek condition. s/HTMLMediaElement can detect/HTMLMediaElement can't detect/
On 2014/09/10 17:44:05, acolwell wrote: > On 2014/09/10 13:52:02, philipj wrote: > > This now LGTM, but I'd like scherkus to take a final look, so that we aren't > > pulling in two directions. To be clear, where I think we should end up is: > > > > * WebMediaPlayer implementation guarantee that currentTime < duration during > > playback and currentTime == duration at playback end. > > This can be problematic especially in the MSE case where JS explicity sets the > duration to a value < the currentTime. Ideally it would be nice if the > WebMediaPlayer just reported what the current time is w/o the duration clamp so > that the Blink code can actually detect that it needs to seek backwards in > response to the duration change. Currently we have hacks for this. Just take a > look at HTMLMediaElement::durationChanged() callers. Oh right, I always forget the MSE cases. What's an end state that would work with MSE? Is it simply to have this mediaEnded() callback and not assert anything about currentTime and duration?
On 2014/09/10 18:49:07, philipj wrote: > On 2014/09/10 17:44:05, acolwell wrote: > > On 2014/09/10 13:52:02, philipj wrote: > > > This now LGTM, but I'd like scherkus to take a final look, so that we aren't > > > pulling in two directions. To be clear, where I think we should end up is: > > > > > > * WebMediaPlayer implementation guarantee that currentTime < duration > during > > > playback and currentTime == duration at playback end. > > > > This can be problematic especially in the MSE case where JS explicity sets the > > duration to a value < the currentTime. Ideally it would be nice if the > > WebMediaPlayer just reported what the current time is w/o the duration clamp > so > > that the Blink code can actually detect that it needs to seek backwards in > > response to the duration change. Currently we have hacks for this. Just take a > > look at HTMLMediaElement::durationChanged() callers. > > Oh right, I always forget the MSE cases. What's an end state that would work > with MSE? Is it simply to have this mediaEnded() callback and not assert > anything about currentTime and duration? I don't have a specific suggestion at the moment. Ideally it would be nice if we had a simple onDurationChange(new_duration) method on HTMLMediaElement and all the logic to deal with whether a seek is needed or not is completely inside HTMLMediaElement. If WebMediaPlayer::currentTime() is truncated by WebMediaPlayer::duration(), like it is now, it makes it difficult for the HTMLMediaElement to actually determine whether a seek is needed or not. I'm just offering this as a counter-argument for encouraging truncating WebMediaPlayer::currentTime() in the impls. You don't necessarily need to take action on it in this CL. Just wanted to bring the issue to people's attention.
Thanks, these are good points, I'll try to remember them this time :) I think this CL is an improvement, so if LGTM'ed by scherkus or acolwell it's good for landing.
Ping scherkus.
amogh: can you rebase and also upload the Chromium-side changes that will accompany this CL? my understanding is we're planning on clamping *duration* upon mediaPlayerEnded() as opposed to clamping *currentTime*? is that correct? if so, I'm interested in how we're going to accomplish that in Chromium land since there might be some technical details that make that challenging :\
(also I apologize for the delay -- this fell off my plate and I appreciate the ping!)
On 2014/09/16 23:48:19, scherkus wrote: > amogh: can you rebase and also upload the Chromium-side changes that will > accompany this CL? > The patch has already been uploaded at https://codereview.chromium.org/533543004. I'll rebase both the patches. > my understanding is we're planning on clamping *duration* upon > mediaPlayerEnded() as opposed to clamping *currentTime*? is that correct? > Yes. > if so, I'm interested in how we're going to accomplish that in Chromium land > since there might be some technical details that make that challenging :\ We are sending durationChanged when playback ends and doing refreshCachedTime here.
Ping! ^_^ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
