Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522463003:
Refactor resource load and resource selection algorithms as per spec (Closed)
|
Created:
5 years ago by Srirama Modified:
4 years, 9 months ago CC:
chromium-reviews, nessy, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews, vcarbune.chromium, wolenetz, ddorwin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor resource load and resource selection algorithms as per spec
In the current code the resource load and resource selection algorithms
were mixed and there are no explicit entry points. Added new functions
invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate
them and partially updated the code to match the spec.
BUG=592396
Committed: https://crrev.com/4563c7c2c70ab5f09a017cad834d995485f75ec0
Cr-Commit-Position: refs/heads/master@{#381203}
Patch Set 1 #Patch Set 2 : Removed TODO comment #
Total comments: 3
Patch Set 3 : Update per spec as per review comments #
Total comments: 4
Patch Set 4 : keep function name as prepareForLoad #
Total comments: 1
Patch Set 5 : Modified as per suggestion #
Total comments: 17
Patch Set 6 : Address review comments #
Total comments: 4
Patch Set 7 : Fixed test failures #
Total comments: 13
Patch Set 8 : Fixed review comments #Patch Set 9 : Removed prepareToPlay and refactored code in call sites #
Total comments: 14
Patch Set 10 : Fixed review comments and rebased to latest #
Total comments: 7
Patch Set 11 : Address review comments #Patch Set 12 : Added tests using internals.idl #
Total comments: 16
Patch Set 13 : address nits #
Total comments: 1
Patch Set 14 : address nits #
Total comments: 1
Patch Set 15 : Make preloadTypeToString a helper function #
Total comments: 1
Patch Set 16 : Move preloadTypeToString to anonymous namespace #Messages
Total messages: 99 (29 generated)
Description was changed from ========== fix BUG= ========== to ========== Remove clearMediaPlayer function The functions clearMediaPlayer and resetMediaPlayerAndMediaSource does more or less samething with a few extra bits in clearMediaPlayer. Removing the clearMediaPlayer infavor of resetMediaPlayerAndMediaSource and refactored the code at the callsites. ==========
Description was changed from ========== Remove clearMediaPlayer function The functions clearMediaPlayer and resetMediaPlayerAndMediaSource does more or less samething with a few extra bits in clearMediaPlayer. Removing the clearMediaPlayer infavor of resetMediaPlayerAndMediaSource and refactored the code at the callsites. ========== to ========== Remove clearMediaPlayer infavor of resetMediaPlayerAndMediaSource The functions clearMediaPlayer and resetMediaPlayerAndMediaSource does more or less samething with a few extra bits in clearMediaPlayer. Removing the clearMediaPlayer infavor of resetMediaPlayerAndMediaSource and refactored the code at the callsites. ==========
This is one more change based on our discussions in https://codereview.chromium.org/1416073002/. PTAL.
srirama.m@samsung.com changed reviewers: + philipj@opera.com
https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: m_pendingActionFlags &= ~LoadMediaResource; This does much less than clearMediaPlayer, is the change really not observable?
The CQ bit was checked by philipj@opera.com to run a CQ dry run
Started a dry run to see if tests still pass.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522463003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522463003/20001
https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: m_pendingActionFlags &= ~LoadMediaResource; On 2015/12/11 15:01:51, philipj wrote: > This does much less than clearMediaPlayer, is the change really not observable? resetMediaPlayerAndMediaSource is internally called in scheduleDelayedAction() and remaining bits are done in prepareForLoad().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: m_pendingActionFlags &= ~LoadMediaResource; On 2015/12/11 15:05:47, Srirama wrote: > On 2015/12/11 15:01:51, philipj wrote: > > This does much less than clearMediaPlayer, is the change really not > observable? > > resetMediaPlayerAndMediaSource is internally called in scheduleDelayedAction() > and remaining bits are done in prepareForLoad(). OK, so assuming that this CL does not change any observable behavior, that the bits left is exactly the code that was previously run, in the same order to the extent that it matters... I don't think this is really any clearer. It's weird that we have both clearMediaPlayer and resetMediaPlayerAndMediaSource, but I was hoping that they could be merged into a method. Otherwise the intent of the code is pretty mysterious. Why forget resource specific text tracks here and in parseAttribute? If anything I would hope that both of these places would be reduced into a single call corresponding to "invoke the media element's media element load algorithm", as the spec puts it. If you have an end state in mind for these changes, maybe I can make sense of the intermediate states :)
On 2015/12/14 14:30:40, philipj wrote: > https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: > m_pendingActionFlags &= ~LoadMediaResource; > On 2015/12/11 15:05:47, Srirama wrote: > > On 2015/12/11 15:01:51, philipj wrote: > > > This does much less than clearMediaPlayer, is the change really not > > observable? > > > > resetMediaPlayerAndMediaSource is internally called in scheduleDelayedAction() > > and remaining bits are done in prepareForLoad(). > > OK, so assuming that this CL does not change any observable behavior, that the > bits left is exactly the code that was previously run, in the same order to the > extent that it matters... > > I don't think this is really any clearer. It's weird that we have both > clearMediaPlayer and resetMediaPlayerAndMediaSource, but I was hoping that they > could be merged into a method. Otherwise the intent of the code is pretty > mysterious. Why forget resource specific text tracks here and in parseAttribute? > If anything I would hope that both of these places would be reduced into a > single call corresponding to "invoke the media element's media element load > algorithm", as the spec puts it. > > If you have an end state in mind for these changes, maybe I can make sense of > the intermediate states :) My intention was to make sure that there are less entry points to clearMediaPlayerAndAudioSourceProviderClientWithoutLocking and reset states here, and as you said add checks similar to assertShadowRootChildren finally. And the code in both places is already as per the spec right? We are calling scheduleDelayedAction with LoadMediaResource which will internally call the prepareForLoad(media element load algorithm).
On 2015/12/14 14:56:24, Srirama wrote: > On 2015/12/14 14:30:40, philipj wrote: > > > https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1522463003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: > > m_pendingActionFlags &= ~LoadMediaResource; > > On 2015/12/11 15:05:47, Srirama wrote: > > > On 2015/12/11 15:01:51, philipj wrote: > > > > This does much less than clearMediaPlayer, is the change really not > > > observable? > > > > > > resetMediaPlayerAndMediaSource is internally called in > scheduleDelayedAction() > > > and remaining bits are done in prepareForLoad(). > > > > OK, so assuming that this CL does not change any observable behavior, that the > > bits left is exactly the code that was previously run, in the same order to > the > > extent that it matters... > > > > I don't think this is really any clearer. It's weird that we have both > > clearMediaPlayer and resetMediaPlayerAndMediaSource, but I was hoping that > they > > could be merged into a method. Otherwise the intent of the code is pretty > > mysterious. Why forget resource specific text tracks here and in > parseAttribute? > > If anything I would hope that both of these places would be reduced into a > > single call corresponding to "invoke the media element's media element load > > algorithm", as the spec puts it. > > > > If you have an end state in mind for these changes, maybe I can make sense of > > the intermediate states :) > > My intention was to make sure that there are less entry points to > clearMediaPlayerAndAudioSourceProviderClientWithoutLocking and reset states > here, and as you said add checks similar to assertShadowRootChildren finally. > And the code in both places is already as per the spec right? We are calling > scheduleDelayedAction with LoadMediaResource which will internally call the > prepareForLoad(media element load algorithm). Well, it's not too far from the spec, but the structure doesn't quite match, and I expect some small differences as a result. For example, there's no explicit entry point to the resource selection algorithm, it's just part of HTMLMediaElement::prepareForLoad(). What I mean is that where the spec says "run the media element load algorithm", the implementation should ideally just be invokeLoadAlgorithm() or something similarly explicit. That would be roughly like the current prepareForLoad(), but it would have to do any extra work that's currently done before the call, like here. So my main problem here is that by expanding clearMediaPlayer() in these places, we're further away from the goal of having explicit entry points to the load algorithm and resource selection algorithm. As an alternative, would it work to let prepareForLoad() to do the things that are currently done before the call? There's already a "Perform the cleanup required for the resource load algorithm to run" section. That would leave HTMLMediaElement::stop(). In that case I think it makes more sense to just reset all state as necessary, but would it be possible to have a shared "reset all interesting state" method that's used both by prepareForLoad() and stop()? It would make sense, because in stop() you do want do clear everything as if you were going to load a new resource, but not actually load a new resource, which is what prepareForLoad() would do in addition. Things may be simpler if you save the networkState at the top of prepareForLoad().
Description was changed from ========== Remove clearMediaPlayer infavor of resetMediaPlayerAndMediaSource The functions clearMediaPlayer and resetMediaPlayerAndMediaSource does more or less samething with a few extra bits in clearMediaPlayer. Removing the clearMediaPlayer infavor of resetMediaPlayerAndMediaSource and refactored the code at the callsites. ========== to ========== Update media element load algorithm as per spec Rename prepareForLoad to invokeLoadAlgorithm as per spec. Also refactor and rename clearMediaPlayer to resetMediaElement. ==========
Thanks. So our ultimate aim is to clean up the code and make it as close to spec as possible. Will keep this in mind whenever i do the changes. https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:507: m_pendingActionFlags |= LoadMediaResource; May be we can move this as well inside, probably while separating resource selection algorithm, i will do it. https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:740: if (networkState != NETWORK_EMPTY) { Most of this section is covered in resetMediaElement, but i left it as it is for time being, so that it will be in sync with the spec.
https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:507: m_pendingActionFlags |= LoadMediaResource; On 2015/12/16 07:33:43, Srirama wrote: > May be we can move this as well inside, probably while separating resource > selection algorithm, i will do it. Yes, unless all of this could be moved inside the call, I wouldn't rename it invokeLoadAlgorithm(). https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:713: invokeLoadAlgorith(); In fact, I would keep the name prepareForLoad() for now, since loadInternal() is still after this call. To really make this per spec, HTMLMediaElement::scheduleDelayedAction would have to be fixed, because that's calling prepareForLoad() without a following loadInternal(), which is like the "media element load algorithm" without the final "Invoke the media element's resource selection algorithm" which isn't something the spec ever does. I wouldn't recommend trying to sort out all of this at the same time, though. Without trying to do the work myself, I can't really suggest which order to fix things to make sense, you'll have to experiment I think.
Description was changed from ========== Update media element load algorithm as per spec Rename prepareForLoad to invokeLoadAlgorithm as per spec. Also refactor and rename clearMediaPlayer to resetMediaElement. ========== to ========== Refactor and rename clearMediaPlayer In prepareForLoad() we clear everything and start a new load. In stop() we clear everything but don't load. So moved all the clearing part to clearMediaPlayer and renamed it to resetMediaElement and used in both the places. ==========
On 2015/12/16 15:51:07, philipj wrote: > https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:507: > m_pendingActionFlags |= LoadMediaResource; > On 2015/12/16 07:33:43, Srirama wrote: > > May be we can move this as well inside, probably while separating resource > > selection algorithm, i will do it. > > Yes, unless all of this could be moved inside the call, I wouldn't rename it > invokeLoadAlgorithm(). > > https://codereview.chromium.org/1522463003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:713: > invokeLoadAlgorith(); > In fact, I would keep the name prepareForLoad() for now, since loadInternal() is > still after this call. > > To really make this per spec, HTMLMediaElement::scheduleDelayedAction would have > to be fixed, because that's calling prepareForLoad() without a following > loadInternal(), which is like the "media element load algorithm" without the > final "Invoke the media element's resource selection algorithm" which isn't > something the spec ever does. scheduleDelayedAction is starting a timer for loadInternal. prepareForLoad starts resource selection algorithm (first 3steps) and then scheduleDelayedAction start load timer. Isn't it as per the spec(4th step in resource selection algorithm)? > I wouldn't recommend trying to sort out all of this at the same time, though. > Without trying to do the work myself, I can't really suggest which order to fix > things to make sense, you'll have to experiment I think. Probably this CL can be landed as is, before i can start other experiments.
https://codereview.chromium.org/1522463003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: prepareForLoad(); I think that this bit is made worse by this CL, expanding scheduleDelayedAction() in order to avoid another call to prepareForLoad(), it seems. Could you try to instead remove prepareForLoad() from scheduleDelayedAction()? I've often been surprised to rediscover that scheduleDelayedAction() actually does something synchronously. Then presumably scheduleNextSourceChild() could simply call scheduleDelayedAction() as well. I don't know exactly how this will all end up, but without a clear idea that the end result is going to be an improvement, I'm reluctant to make it temporarily worse.
Patchset #5 (id:80001) has been deleted
On 2015/12/18 15:12:24, philipj wrote: > https://codereview.chromium.org/1522463003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:474: prepareForLoad(); > I think that this bit is made worse by this CL, expanding > scheduleDelayedAction() in order to avoid another call to prepareForLoad(), it > seems. > > Could you try to instead remove prepareForLoad() from scheduleDelayedAction()? > I've often been surprised to rediscover that scheduleDelayedAction() actually > does something synchronously. Then presumably scheduleNextSourceChild() could > simply call scheduleDelayedAction() as well. > > I don't know exactly how this will all end up, but without a clear idea that the > end result is going to be an improvement, I'm reluctant to make it temporarily > worse. PTAL. Probably some more refinement? Rename scheduleNextSourceChild to a more general name?
The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522463003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522463003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
It's been a long time, a lot of vacation over Christmas for me. I've taken a new look as if I'd never seen this code before, hope I'm not repeating old points. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:799: // 2 - Asynchronously await a stable state. Why was this comment removed? The spec presumably said this when this code was implemented, and it's still not quite per spec with regard to awaiting a stable state. If you remove this, instead sync with the new spec steps and add TODOs for the bits that aren't correct. Probably at the end of this method is where you should schedule an async continuation corresponding to "await a stable state". (That should use a microtask per spec, but let's try to not introduce any observable changes with this cleanup.) https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:504: prepareForLoad(); The spec says "If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm." If you want to wrap these two in a new invokeLoadAlgorithm() that might be nice. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:543: if (!(m_pendingActionFlags & LoadMediaResource)) I'm not sure I understand these changes. The existing behavior is not per spec, as inserting a media element into a document shouldn't actually do anything. IIRC, this is actually needed here because removing from a document will destroy the media player. Would it work to just invoke the resource selection algorithm here? Or the media element load algorithm? It also doesn't really make sense that this is looking at the src attribute, when there could also be source element children. If the reason for that is that inserting a source element child will invoke the resource selection algorithm, then I think doing the same here makes the most sense. Please also decorate this with a TODO explaining why it's actually wrong and not per spec. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:598: if (!m_loadTimer.isActive()) Why was this change needed? https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:720: resetMediaElement(LoadMediaResource); This will now be the only place where resetMediaElement is called with an argument other than -1. Isn't it possible to just always clear all flags here? Any async continuation of this method should be scheduled after this point. If you make that change, also make sure to cancel any pending callback. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1974: if (m_networkState == NETWORK_EMPTY) { Can this simply be invokeResourceSelectionAlgorithm() as in the spec? If that change has any observable side-effects, a TODO explaining the difference and how to remove it would be good. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2039: if (!(m_pendingActionFlags & LoadMediaResource)) Same as for playInternal(). https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2653: if (!(m_pendingActionFlags & LoadMediaResource)) Here too, can we just invoke the resource selection algorithm? https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (left): https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:95: enum DelayedActionType { Maybe move this down to m_pendingActionFlags and call if PendingAction or something?
On 2016/01/21 12:48:11, philipj_OOO_til_Jan25 wrote: > It's been a long time, a lot of vacation over Christmas for me. I've taken a new > look as if I'd never seen this code before, hope I'm not repeating old points. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:799: // 2 - > Asynchronously await a stable state. > Why was this comment removed? The spec presumably said this when this code was > implemented, and it's still not quite per spec with regard to awaiting a stable > state. If you remove this, instead sync with the new spec steps and add TODOs > for the bits that aren't correct. > > Probably at the end of this method is where you should schedule an async > continuation corresponding to "await a stable state". (That should use a > microtask per spec, but let's try to not introduce any observable changes with > this cleanup.) > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:504: prepareForLoad(); > The spec says "If a src attribute of a media element is set or changed, the user > agent must invoke the media element's media element load algorithm." If you want > to wrap these two in a new invokeLoadAlgorithm() that might be nice. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:543: if > (!(m_pendingActionFlags & LoadMediaResource)) > I'm not sure I understand these changes. The existing behavior is not per spec, > as inserting a media element into a document shouldn't actually do anything. > IIRC, this is actually needed here because removing from a document will destroy > the media player. Would it work to just invoke the resource selection algorithm > here? Or the media element load algorithm? > > It also doesn't really make sense that this is looking at the src attribute, > when there could also be source element children. If the reason for that is that > inserting a source element child will invoke the resource selection algorithm, > then I think doing the same here makes the most sense. Please also decorate this > with a TODO explaining why it's actually wrong and not per spec. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:598: if > (!m_loadTimer.isActive()) > Why was this change needed? > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:720: > resetMediaElement(LoadMediaResource); > This will now be the only place where resetMediaElement is called with an > argument other than -1. Isn't it possible to just always clear all flags here? > Any async continuation of this method should be scheduled after this point. > > If you make that change, also make sure to cancel any pending callback. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1974: if > (m_networkState == NETWORK_EMPTY) { > Can this simply be invokeResourceSelectionAlgorithm() as in the spec? If that > change has any observable side-effects, a TODO explaining the difference and how > to remove it would be good. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2039: if > (!(m_pendingActionFlags & LoadMediaResource)) > Same as for playInternal(). > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2653: if > (!(m_pendingActionFlags & LoadMediaResource)) > Here too, can we just invoke the resource selection algorithm? > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.h (left): > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.h:95: enum > DelayedActionType { > Maybe move this down to m_pendingActionFlags and call if PendingAction or > something? Even i am on a business trip, so i didn't ping you. I am back today and i will work on these review comments.
https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:799: // 2 - Asynchronously await a stable state. On 2016/01/21 12:48:11, philipj_UTC7 wrote: > Why was this comment removed? The spec presumably said this when this code was > implemented, and it's still not quite per spec with regard to awaiting a stable > state. If you remove this, instead sync with the new spec steps and add TODOs > for the bits that aren't correct. > > Probably at the end of this method is where you should schedule an async > continuation corresponding to "await a stable state". (That should use a > microtask per spec, but let's try to not introduce any observable changes with > this cleanup.) Done. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:504: prepareForLoad(); On 2016/01/21 12:48:11, philipj_UTC7 wrote: > The spec says "If a src attribute of a media element is set or changed, the user > agent must invoke the media element's media element load algorithm." If you want > to wrap these two in a new invokeLoadAlgorithm() that might be nice. Done. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:543: if (!(m_pendingActionFlags & LoadMediaResource)) On 2016/01/21 12:48:11, philipj_UTC7 wrote: > I'm not sure I understand these changes. The existing behavior is not per spec, > as inserting a media element into a document shouldn't actually do anything. > IIRC, this is actually needed here because removing from a document will destroy > the media player. Would it work to just invoke the resource selection algorithm > here? Or the media element load algorithm? > > It also doesn't really make sense that this is looking at the src attribute, > when there could also be source element children. If the reason for that is that > inserting a source element child will invoke the resource selection algorithm, > then I think doing the same here makes the most sense. Please also decorate this > with a TODO explaining why it's actually wrong and not per spec. Removed unneeded conditions. But i couldn't exactly figure the spec location for this, i could see the spec corresponding to source element insertion but couldn't see the same thing for media element. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:598: if (!m_loadTimer.isActive()) On 2016/01/21 12:48:11, philipj_UTC7 wrote: > Why was this change needed? Removed, not needed, i have added it to fix layout test case failures, but i think those failures may be because of the changes in above places. After fixing your review comments all the test cases are passing. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:720: resetMediaElement(LoadMediaResource); On 2016/01/21 12:48:10, philipj_UTC7 wrote: > This will now be the only place where resetMediaElement is called with an > argument other than -1. Isn't it possible to just always clear all flags here? > Any async continuation of this method should be scheduled after this point. > > If you make that change, also make sure to cancel any pending callback. passing -1 is resetting all the flags and because of that text track related test cases are failing. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1974: if (m_networkState == NETWORK_EMPTY) { On 2016/01/21 12:48:11, philipj_UTC7 wrote: > Can this simply be invokeResourceSelectionAlgorithm() as in the spec? If that > change has any observable side-effects, a TODO explaining the difference and how > to remove it would be good. Done. Do you want me to remove scheduleNextSourceChild() here? Removing scheduleNextSourceChild() is causing test failure. So keeping it for now. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2039: if (!(m_pendingActionFlags & LoadMediaResource)) On 2016/01/21 12:48:11, philipj_UTC7 wrote: > Same as for playInternal(). Done. https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2653: if (!(m_pendingActionFlags & LoadMediaResource)) On 2016/01/21 12:48:11, philipj_UTC7 wrote: > Here too, can we just invoke the resource selection algorithm? Done.
On 2016/01/28 11:31:03, Srirama wrote: > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:799: // 2 - > Asynchronously await a stable state. > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > Why was this comment removed? The spec presumably said this when this code was > > implemented, and it's still not quite per spec with regard to awaiting a > stable > > state. If you remove this, instead sync with the new spec steps and add TODOs > > for the bits that aren't correct. > > > > Probably at the end of this method is where you should schedule an async > > continuation corresponding to "await a stable state". (That should use a > > microtask per spec, but let's try to not introduce any observable changes with > > this cleanup.) > > Done. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:504: prepareForLoad(); > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > The spec says "If a src attribute of a media element is set or changed, the > user > > agent must invoke the media element's media element load algorithm." If you > want > > to wrap these two in a new invokeLoadAlgorithm() that might be nice. > > Done. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:543: if > (!(m_pendingActionFlags & LoadMediaResource)) > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > I'm not sure I understand these changes. The existing behavior is not per > spec, > > as inserting a media element into a document shouldn't actually do anything. > > IIRC, this is actually needed here because removing from a document will > destroy > > the media player. Would it work to just invoke the resource selection > algorithm > > here? Or the media element load algorithm? > > > > It also doesn't really make sense that this is looking at the src attribute, > > when there could also be source element children. If the reason for that is > that > > inserting a source element child will invoke the resource selection algorithm, > > then I think doing the same here makes the most sense. Please also decorate > this > > with a TODO explaining why it's actually wrong and not per spec. > > Removed unneeded conditions. But i couldn't exactly figure the spec location for > this, i could see the spec corresponding to source element insertion but > couldn't see the same thing for media element. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:598: if > (!m_loadTimer.isActive()) > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > Why was this change needed? > > Removed, not needed, i have added it to fix layout test case failures, but i > think those failures may be because of the changes in above places. After fixing > your review comments all the test cases are passing. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:720: > resetMediaElement(LoadMediaResource); > On 2016/01/21 12:48:10, philipj_UTC7 wrote: > > This will now be the only place where resetMediaElement is called with an > > argument other than -1. Isn't it possible to just always clear all flags here? > > Any async continuation of this method should be scheduled after this point. > > > > If you make that change, also make sure to cancel any pending callback. > > passing -1 is resetting all the flags and because of that text track related > test cases are failing. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1974: if > (m_networkState == NETWORK_EMPTY) { > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > Can this simply be invokeResourceSelectionAlgorithm() as in the spec? If that > > change has any observable side-effects, a TODO explaining the difference and > how > > to remove it would be good. > > Done. Do you want me to remove scheduleNextSourceChild() here? Removing > scheduleNextSourceChild() is causing test failure. So keeping it for now. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2039: if > (!(m_pendingActionFlags & LoadMediaResource)) > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > Same as for playInternal(). > > Done. > > https://codereview.chromium.org/1522463003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2653: if > (!(m_pendingActionFlags & LoadMediaResource)) > On 2016/01/21 12:48:11, philipj_UTC7 wrote: > > Here too, can we just invoke the resource selection algorithm? > > Done. @Philip, there are some encrypted media test cases failing. I made a local build by enabling the proprietary codecs and tried running those test cases but there seems to be some problem. Looks like the request itself is failing. [31344:31457:0129/200317:WARNING:embedded_test_server.cc(167)] Request not handled. Returning 404: /mse_different_containers.html?keySystem=org.w3.clearkey&runEncrypted=1&videoFormat=CLEAR_WEBM&audioFormat=ENCRYPTED_MP4 Any idea how to run these tests locally?
On 2016/01/29 14:55:10, Srirama wrote: > @Philip, there are some encrypted media test cases failing. I made a local build > by enabling the proprietary codecs and tried running those test cases but there > seems to be some problem. Looks like the request itself is failing. > [31344:31457:0129/200317:WARNING:embedded_test_server.cc(167)] Request not > handled. Returning 404: > /mse_different_containers.html?keySystem=org.w3.clearkey&runEncrypted=1&videoFormat=CLEAR_WEBM&audioFormat=ENCRYPTED_MP4 > > Any idea how to run these tests locally? Hmm, that's in chromium/src/media/test/data/mse_different_containers.html, are you running the right server to serve from that directory? I'd ask media/OWNERS for advice if you can't find a solution.
On 2016/02/02 09:58:35, philipj_UTC7 wrote: > On 2016/01/29 14:55:10, Srirama wrote: > > > @Philip, there are some encrypted media test cases failing. I made a local > build > > by enabling the proprietary codecs and tried running those test cases but > there > > seems to be some problem. Looks like the request itself is failing. > > [31344:31457:0129/200317:WARNING:embedded_test_server.cc(167)] Request not > > handled. Returning 404: > > > /mse_different_containers.html?keySystem=org.w3.clearkey&runEncrypted=1&videoFormat=CLEAR_WEBM&audioFormat=ENCRYPTED_MP4 > > > > Any idea how to run these tests locally? > > Hmm, that's in chromium/src/media/test/data/mse_different_containers.html, are > you running the right server to serve from that directory? > > I'd ask media/OWNERS for advice if you can't find a solution. I picked up the command from the failed bot logs. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium.... Do we need to separately run the server in this case? I guess probably those media test files should be downloaded before running the test cases, something like perf test cases. I would be helpful if we can get a hint from the media/OWNERS.
philipj@opera.com changed reviewers: + ddorwin@chromium.org
Not sure if you reached out via email, but otherwise, here's ddorwin@ who knows everything worth knowing about EME!
On 2016/02/02 16:40:09, philipj_UTC7 wrote: > Not sure if you reached out via email, but otherwise, here's ddorwin@ who knows > everything worth knowing about EME! @ddorwin, there are some EME test failures in browser_tests(ex: MSE_ClearKey_EncryptedMediaTest.Playback_ClearVideo_WEBM_EncryptedAudio_MP4_0) which i was trying to debug locally. So i made a local build with  "proprietary_codecs=1" in gyp defines and tried to run the test case but i get a 404 error [31344:31457:0129/200317:WARNING:embedded_test_server.cc(167)] Request not handled. Returning 404: /mse_different_containers.html?keySystem=org.w3.clearkey&runEncrypted=1&videoFormat=CLEAR_WEBM&audioFormat=ENCRYPTED_MP4. Is there anything else i need to do? Do i need to add "ffmpeg_branding=Chrome" also to gyp defines?
The set of tests failing all use TestDifferentContainers [1]. You can see that the names include both WEBM and MP4. There are similar tests for MSE alone [2] that are passing. I suspect something is breaking playback of different containers with EME, though I don't know why that would be. I left a comment on part of the change that I wasn't sure about. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... On 2016/02/02 17:50:15, Srirama wrote: > On 2016/02/02 16:40:09, philipj_UTC7 wrote: > > Not sure if you reached out via email, but otherwise, here's ddorwin@ who > knows > > everything worth knowing about EME! > > @ddorwin, there are some EME test failures in browser_tests(ex: > MSE_ClearKey_EncryptedMediaTest.Playback_ClearVideo_WEBM_EncryptedAudio_MP4_0) > which i was trying to debug locally. So i made a local build with > "proprietary_codecs=1" in gyp defines and tried to run the test case but i get > a 404 error > [31344:31457:0129/200317:WARNING:embedded_test_server.cc(167)] Request not > handled. Returning 404: > /mse_different_containers.html?keySystem=org.w3.clearkey&runEncrypted=1&videoFormat=CLEAR_WEBM&audioFormat=ENCRYPTED_MP4. > Is there anything else i need to do? Do i need to add "ffmpeg_branding=Chrome" > also to gyp defines? Yes, you need to set both proprietary_codecs=1 and ffmpeg_branding=Chrome. (You can see the full configuration at https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...)
https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:462: invokeLoadAlgorithm(); I don't know if this is related, but you are no longer calling resetMediaElement() here.
ddorwin@chromium.org changed reviewers: + jrummell@chromium.org - ddorwin@chromium.org
https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:529: invokeResourceSelectionAlgorithm(); Is this correct? previous calls to scheduleDelayedAction(LoadMediaResource) are changed to invokeLoadAlgorithm(); https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:581: m_loadTimer.startOneShot(0, BLINK_FROM_HERE); The previous scheduleDelayedAction() only started the timer if it wasn't active. Since this is now called more often, so it have the check "if (!m_loadTimer.isActive()) ..."? https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:701: prepareForLoad(); The previous scheduleDelayedAction() only called prepareForLoad() if m_pendingActionFlags didn't have the flag set. Is that required here? Not sure if calling prepareForLoad() multipel times will cause problems or not.
On 2016/02/02 21:39:08, jrummell wrote: > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:529: > invokeResourceSelectionAlgorithm(); > Is this correct? previous calls to scheduleDelayedAction(LoadMediaResource) are > changed to invokeLoadAlgorithm(); > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:581: > m_loadTimer.startOneShot(0, BLINK_FROM_HERE); > The previous scheduleDelayedAction() only started the timer if it wasn't active. > Since this is now called more often, so it have the check "if > (!m_loadTimer.isActive()) ..."? > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:701: prepareForLoad(); > The previous scheduleDelayedAction() only called prepareForLoad() if > m_pendingActionFlags didn't have the flag set. Is that required here? Not sure > if calling prepareForLoad() multipel times will cause problems or not. @jrummel, you are right, there is a change from the existing code, we are trying to align to spec as much as possible and cleanup the unnecessary code. All layout tests are passing but these EME test cases are failing. I have tried the local build using the configuration from the build bot. But When i run the test case locally, as i mentioned earlier i get 404 error and the test timesout. Please let me know how to run these tests in a local linux build.
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
Description was changed from ========== Refactor and rename clearMediaPlayer In prepareForLoad() we clear everything and start a new load. In stop() we clear everything but don't load. So moved all the clearing part to clearMediaPlayer and renamed it to resetMediaElement and used in both the places. ========== to ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. ==========
On 2016/02/03 12:54:18, Srirama wrote: > On 2016/02/02 21:39:08, jrummell wrote: > > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:529: > > invokeResourceSelectionAlgorithm(); > > Is this correct? previous calls to scheduleDelayedAction(LoadMediaResource) > are > > changed to invokeLoadAlgorithm(); > > > > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:581: > > m_loadTimer.startOneShot(0, BLINK_FROM_HERE); > > The previous scheduleDelayedAction() only started the timer if it wasn't > active. > > Since this is now called more often, so it have the check "if > > (!m_loadTimer.isActive()) ..."? > > > > > https://codereview.chromium.org/1522463003/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:701: > prepareForLoad(); > > The previous scheduleDelayedAction() only called prepareForLoad() if > > m_pendingActionFlags didn't have the flag set. Is that required here? Not sure > > if calling prepareForLoad() multipel times will cause problems or not. > > @jrummel, you are right, there is a change from the existing code, we are trying > to align to spec as much as possible and cleanup the unnecessary code. All > layout tests are passing but these EME test cases are failing. I have tried the > local build using the configuration from the build bot. But When i run the test > case locally, as i mentioned earlier i get 404 error and the test timesout. > Please let me know how to run these tests in a local linux build. @Philip, can you please have a look one more time. Looks like the previous EME test failures are due to the refactoring of clearMediaPlayer function. I have removed those changes now, probably i can experiment that in a separate CL.
OK, it was a good move to deal only with the algorithm refactoring in this CL. I think this direction is very good, but have commented on mismatches between spec and implementation that I could spot. I think that it should be possible to get this right in a single CL, but I haven't tried it myself, so if there's something that doesn't seem to work just shout at me. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:462: clearMediaPlayer(LoadMediaResource); As I wrote in some earlier CL, I think this belongs inside invokeLoadAlgorithm(), so that it's always just a single call where the spec says "invoke the media element load algorithm", although this isn't one of those places. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:492: clearMediaPlayer(LoadMediaResource); Here too. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: scheduleNextSourceChild(); scheduleNextSourceChild() belongs somewhere in the resource selection algorithm. If that move works, you can simply rename prepareForLoad() to invokeLoadAlgorithm() which would make for one less internal concept to understand. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:810: // TODO(srirama.m): Schedule an async continuation of this algorithm as per the spec How does this currently work? I think it's the earlier scheduleNextSourceChild() that should be moved here. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1979: scheduleNextSourceChild(); Try to get rid of scheduleNextSourceChild() here and below. Traversing source children is what the resource selection algorithm does, having scheduleNextSourceChild() *after* the invocation here is a sign of some mismatch.
https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:462: clearMediaPlayer(LoadMediaResource); On 2016/02/09 07:34:34, philipj_UTC7 wrote: > As I wrote in some earlier CL, I think this belongs inside > invokeLoadAlgorithm(), so that it's always just a single call where the spec > says "invoke the media element load algorithm", although this isn't one of those > places. Done. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:492: clearMediaPlayer(LoadMediaResource); On 2016/02/09 07:34:34, philipj_UTC7 wrote: > Here too. Done. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: scheduleNextSourceChild(); On 2016/02/09 07:34:34, philipj_UTC7 wrote: > scheduleNextSourceChild() belongs somewhere in the resource selection algorithm. > If that move works, you can simply rename prepareForLoad() to > invokeLoadAlgorithm() which would make for one less internal concept to > understand. As you said in the below comment, we can move scheduleNextSourceChild() inside invokeResourceSelectionAlgorithm() but the load() function is where it may create problem. There we directly invoke loadInternal() and then prepareToPlay() without using scheduleNextSourceChild(). So what should we do about prepareToPlay()? Should we remove that and just invoke resource load algorithm as the spec says? https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:810: // TODO(srirama.m): Schedule an async continuation of this algorithm as per the spec On 2016/02/09 07:34:34, philipj_UTC7 wrote: > How does this currently work? I think it's the earlier scheduleNextSourceChild() > that should be moved here. We can move it expect for the problem mentioned in my above comment. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1979: scheduleNextSourceChild(); On 2016/02/09 07:34:34, philipj_UTC7 wrote: > Try to get rid of scheduleNextSourceChild() here and below. > > Traversing source children is what the resource selection algorithm does, having > scheduleNextSourceChild() *after* the invocation here is a sign of some > mismatch. Probably this is also linked to the above issue.
https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: scheduleNextSourceChild(); On 2016/02/09 14:50:46, Srirama wrote: > On 2016/02/09 07:34:34, philipj_UTC7 wrote: > > scheduleNextSourceChild() belongs somewhere in the resource selection > algorithm. > > If that move works, you can simply rename prepareForLoad() to > > invokeLoadAlgorithm() which would make for one less internal concept to > > understand. > > As you said in the below comment, we can move scheduleNextSourceChild() inside > invokeResourceSelectionAlgorithm() but the load() function is where it may > create problem. There we directly invoke loadInternal() and then prepareToPlay() > without using scheduleNextSourceChild(). So what should we do about > prepareToPlay()? Should we remove that and just invoke resource load algorithm > as the spec says? OK, this is interesting. Per spec load() absolutely should invoke the media element load algorithm, in fact it is the only thing it should do. In addition, there's an extra prepareToPlay() call, and that is needed so that preload="none" is overridden. The spec bug is https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten around to updating the spec for that yet. It looks like prepareToPlay() has to be called after loadInternal(), because before that there may not be a media resource to act on in HTMLMediaElement::executeDeferredLoad. The cleanest approach would be to make sure that "Delaying load because preload == 'none'" path in HTMLMediaElement::loadResource is not taken if load() was called, and currently that depends on m_havePreparedToPlay. I poked at it in https://codereview.chromium.org/1687793002 and it would be nice if only effectivePreloadType() were used there. So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay to something that has the semantics of "we will now ignore preload=none, clamping the effective preload state to metadata or higher depending on other state". Not sure what to call it, maybe you can see if it would actually work first?
https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: scheduleNextSourceChild(); On 2016/02/10 05:02:30, philipj_OOO_til_Feb15 wrote: > On 2016/02/09 14:50:46, Srirama wrote: > > On 2016/02/09 07:34:34, philipj_UTC7 wrote: > > > scheduleNextSourceChild() belongs somewhere in the resource selection > > algorithm. > > > If that move works, you can simply rename prepareForLoad() to > > > invokeLoadAlgorithm() which would make for one less internal concept to > > > understand. > > > > As you said in the below comment, we can move scheduleNextSourceChild() inside > > invokeResourceSelectionAlgorithm() but the load() function is where it may > > create problem. There we directly invoke loadInternal() and then > prepareToPlay() > > without using scheduleNextSourceChild(). So what should we do about > > prepareToPlay()? Should we remove that and just invoke resource load algorithm > > as the spec says? > > OK, this is interesting. Per spec load() absolutely should invoke the media > element load algorithm, in fact it is the only thing it should do. > > In addition, there's an extra prepareToPlay() call, and that is needed so that > preload="none" is overridden. The spec bug is > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten around > to updating the spec for that yet. > > It looks like prepareToPlay() has to be called after loadInternal(), because > before that there may not be a media resource to act on in > HTMLMediaElement::executeDeferredLoad. > > The cleanest approach would be to make sure that "Delaying load because preload > == 'none'" path in HTMLMediaElement::loadResource is not taken if load() was > called, and currently that depends on m_havePreparedToPlay. I poked at it in > https://codereview.chromium.org/1687793002 and it would be nice if only > effectivePreloadType() were used there. > > So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay to > something that has the semantics of "we will now ignore preload=none, clamping > the effective preload state to metadata or higher depending on other state". Not > sure what to call it, maybe you can see if it would actually work first? ok, i have tried the changes locally and all the layout tests are passing. But one problem here is the flag m_havePreparedToPlay is currently being reset in prepareForLoad(), so right now if i set the flag inbetween prepareforload and loadinternal it is working fine. But once we remove all this infavor of invokeLoadAlgorithm, how can we handle this flag? Should i reset the flag at the call sites ?
On 2016/02/10 11:16:19, Srirama wrote: > https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: > scheduleNextSourceChild(); > On 2016/02/10 05:02:30, philipj_OOO_til_Feb15 wrote: > > On 2016/02/09 14:50:46, Srirama wrote: > > > On 2016/02/09 07:34:34, philipj_UTC7 wrote: > > > > scheduleNextSourceChild() belongs somewhere in the resource selection > > > algorithm. > > > > If that move works, you can simply rename prepareForLoad() to > > > > invokeLoadAlgorithm() which would make for one less internal concept to > > > > understand. > > > > > > As you said in the below comment, we can move scheduleNextSourceChild() > inside > > > invokeResourceSelectionAlgorithm() but the load() function is where it may > > > create problem. There we directly invoke loadInternal() and then > > prepareToPlay() > > > without using scheduleNextSourceChild(). So what should we do about > > > prepareToPlay()? Should we remove that and just invoke resource load > algorithm > > > as the spec says? > > > > OK, this is interesting. Per spec load() absolutely should invoke the media > > element load algorithm, in fact it is the only thing it should do. > > > > In addition, there's an extra prepareToPlay() call, and that is needed so that > > preload="none" is overridden. The spec bug is > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten > around > > to updating the spec for that yet. > > > > It looks like prepareToPlay() has to be called after loadInternal(), because > > before that there may not be a media resource to act on in > > HTMLMediaElement::executeDeferredLoad. > > > > The cleanest approach would be to make sure that "Delaying load because > preload > > == 'none'" path in HTMLMediaElement::loadResource is not taken if load() was > > called, and currently that depends on m_havePreparedToPlay. I poked at it in > > https://codereview.chromium.org/1687793002 and it would be nice if only > > effectivePreloadType() were used there. > > > > So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay to > > something that has the semantics of "we will now ignore preload=none, clamping > > the effective preload state to metadata or higher depending on other state". > Not > > sure what to call it, maybe you can see if it would actually work first? > > ok, i have tried the changes locally and all the layout tests are passing. But > one problem here is the flag m_havePreparedToPlay is currently being reset in > prepareForLoad(), so right now if i set the flag inbetween prepareforload and > loadinternal it is working fine. But once we remove all this infavor of > invokeLoadAlgorithm, how can we handle this flag? Should i reset the flag at the > call sites ? Interestingly removing m_havePreparedToPlay = false; from prepareforload also passes all the test cases.
I've been OOO for a bit, sorry for the delay. https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: scheduleNextSourceChild(); On 2016/02/10 11:16:19, Srirama wrote: > On 2016/02/10 05:02:30, philipj_OOO_til_Feb15 wrote: > > On 2016/02/09 14:50:46, Srirama wrote: > > > On 2016/02/09 07:34:34, philipj_UTC7 wrote: > > > > scheduleNextSourceChild() belongs somewhere in the resource selection > > > algorithm. > > > > If that move works, you can simply rename prepareForLoad() to > > > > invokeLoadAlgorithm() which would make for one less internal concept to > > > > understand. > > > > > > As you said in the below comment, we can move scheduleNextSourceChild() > inside > > > invokeResourceSelectionAlgorithm() but the load() function is where it may > > > create problem. There we directly invoke loadInternal() and then > > prepareToPlay() > > > without using scheduleNextSourceChild(). So what should we do about > > > prepareToPlay()? Should we remove that and just invoke resource load > algorithm > > > as the spec says? > > > > OK, this is interesting. Per spec load() absolutely should invoke the media > > element load algorithm, in fact it is the only thing it should do. > > > > In addition, there's an extra prepareToPlay() call, and that is needed so that > > preload="none" is overridden. The spec bug is > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten > around > > to updating the spec for that yet. > > > > It looks like prepareToPlay() has to be called after loadInternal(), because > > before that there may not be a media resource to act on in > > HTMLMediaElement::executeDeferredLoad. > > > > The cleanest approach would be to make sure that "Delaying load because > preload > > == 'none'" path in HTMLMediaElement::loadResource is not taken if load() was > > called, and currently that depends on m_havePreparedToPlay. I poked at it in > > https://codereview.chromium.org/1687793002 and it would be nice if only > > effectivePreloadType() were used there. > > > > So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay to > > something that has the semantics of "we will now ignore preload=none, clamping > > the effective preload state to metadata or higher depending on other state". > Not > > sure what to call it, maybe you can see if it would actually work first? > > ok, i have tried the changes locally and all the layout tests are passing. But > one problem here is the flag m_havePreparedToPlay is currently being reset in > prepareForLoad(), so right now if i set the flag inbetween prepareforload and > loadinternal it is working fine. But once we remove all this infavor of > invokeLoadAlgorithm, how can we handle this flag? Should i reset the flag at the > call sites ? Let's see, m_havePreparedToPlay is used in two ways: 1. It's combined with effectivePreloadType() to possibly defer the load in HTMLMediaElement::loadResource, which is very much related to https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 2. It can cause an early return in HTMLMediaElement::prepareToPlay(), and removing that could cause startDeferredLoad() to be called twice if loadIsDeferred(), but judging by the various methods involved in load deferral, it looks like perhaps this isn't observable at all. So, I would propose removing prepareToPlay() and m_havePreparedToPlay and replacing them with two things that explicitly do what seems to be the important bits: 1. A flag which causes effectivePreloadType() to return PreloadAuto if it is set, and it is set by playing and seeking. 2. Whatever changes, if any, are required to ensure that a deferred load is started precisely when it should be, at least when seeking and playing. May require some changes to the whole deferred load machinery.
On 2016/02/16 12:19:34, philipj_UTC7 wrote: > I've been OOO for a bit, sorry for the delay. No Problem. > > https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: > scheduleNextSourceChild(); > On 2016/02/10 11:16:19, Srirama wrote: > > On 2016/02/10 05:02:30, philipj_OOO_til_Feb15 wrote: > > > On 2016/02/09 14:50:46, Srirama wrote: > > > > On 2016/02/09 07:34:34, philipj_UTC7 wrote: > > > > > scheduleNextSourceChild() belongs somewhere in the resource selection > > > > algorithm. > > > > > If that move works, you can simply rename prepareForLoad() to > > > > > invokeLoadAlgorithm() which would make for one less internal concept to > > > > > understand. > > > > > > > > As you said in the below comment, we can move scheduleNextSourceChild() > > inside > > > > invokeResourceSelectionAlgorithm() but the load() function is where it may > > > > create problem. There we directly invoke loadInternal() and then > > > prepareToPlay() > > > > without using scheduleNextSourceChild(). So what should we do about > > > > prepareToPlay()? Should we remove that and just invoke resource load > > algorithm > > > > as the spec says? > > > > > > OK, this is interesting. Per spec load() absolutely should invoke the media > > > element load algorithm, in fact it is the only thing it should do. > > > > > > In addition, there's an extra prepareToPlay() call, and that is needed so > that > > > preload="none" is overridden. The spec bug is > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten > > around > > > to updating the spec for that yet. > > > > > > It looks like prepareToPlay() has to be called after loadInternal(), because > > > before that there may not be a media resource to act on in > > > HTMLMediaElement::executeDeferredLoad. > > > > > > The cleanest approach would be to make sure that "Delaying load because > > preload > > > == 'none'" path in HTMLMediaElement::loadResource is not taken if load() was > > > called, and currently that depends on m_havePreparedToPlay. I poked at it in > > > https://codereview.chromium.org/1687793002 and it would be nice if only > > > effectivePreloadType() were used there. > > > > > > So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay > to > > > something that has the semantics of "we will now ignore preload=none, > clamping > > > the effective preload state to metadata or higher depending on other state". > > Not > > > sure what to call it, maybe you can see if it would actually work first? > > > > ok, i have tried the changes locally and all the layout tests are passing. But > > one problem here is the flag m_havePreparedToPlay is currently being reset in > > prepareForLoad(), so right now if i set the flag inbetween prepareforload and > > loadinternal it is working fine. But once we remove all this infavor of > > invokeLoadAlgorithm, how can we handle this flag? Should i reset the flag at > the > > call sites ? > > Let's see, m_havePreparedToPlay is used in two ways: > > 1. It's combined with effectivePreloadType() to possibly defer the load in > HTMLMediaElement::loadResource, which is very much related to > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 > > 2. It can cause an early return in HTMLMediaElement::prepareToPlay(), and > removing that could cause startDeferredLoad() to be called twice if > loadIsDeferred(), but judging by the various methods involved in load deferral, > it looks like perhaps this isn't observable at all. > > So, I would propose removing prepareToPlay() and m_havePreparedToPlay and > replacing them with two things that explicitly do what seems to be the important > bits: > > 1. A flag which causes effectivePreloadType() to return PreloadAuto if it is > set, and it is set by playing and seeking. > > 2. Whatever changes, if any, are required to ensure that a deferred load is > started precisely when it should be, at least when seeking and playing. May > require some changes to the whole deferred load machinery. I will work on these suggestions
On 2016/02/16 13:56:19, Srirama wrote: > On 2016/02/16 12:19:34, philipj_UTC7 wrote: > > I've been OOO for a bit, sorry for the delay. > No Problem. > > > > > https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1522463003/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:703: > > scheduleNextSourceChild(); > > On 2016/02/10 11:16:19, Srirama wrote: > > > On 2016/02/10 05:02:30, philipj_OOO_til_Feb15 wrote: > > > > On 2016/02/09 14:50:46, Srirama wrote: > > > > > On 2016/02/09 07:34:34, philipj_UTC7 wrote: > > > > > > scheduleNextSourceChild() belongs somewhere in the resource selection > > > > > algorithm. > > > > > > If that move works, you can simply rename prepareForLoad() to > > > > > > invokeLoadAlgorithm() which would make for one less internal concept > to > > > > > > understand. > > > > > > > > > > As you said in the below comment, we can move scheduleNextSourceChild() > > > inside > > > > > invokeResourceSelectionAlgorithm() but the load() function is where it > may > > > > > create problem. There we directly invoke loadInternal() and then > > > > prepareToPlay() > > > > > without using scheduleNextSourceChild(). So what should we do about > > > > > prepareToPlay()? Should we remove that and just invoke resource load > > > algorithm > > > > > as the spec says? > > > > > > > > OK, this is interesting. Per spec load() absolutely should invoke the > media > > > > element load algorithm, in fact it is the only thing it should do. > > > > > > > > In addition, there's an extra prepareToPlay() call, and that is needed so > > that > > > > preload="none" is overridden. The spec bug is > > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 but I haven't gotten > > > around > > > > to updating the spec for that yet. > > > > > > > > It looks like prepareToPlay() has to be called after loadInternal(), > because > > > > before that there may not be a media resource to act on in > > > > HTMLMediaElement::executeDeferredLoad. > > > > > > > > The cleanest approach would be to make sure that "Delaying load because > > > preload > > > > == 'none'" path in HTMLMediaElement::loadResource is not taken if load() > was > > > > called, and currently that depends on m_havePreparedToPlay. I poked at it > in > > > > https://codereview.chromium.org/1687793002 and it would be nice if only > > > > effectivePreloadType() were used there. > > > > > > > > So, I would lean towards renaming prepareToPlay() and m_havePreparedToPlay > > to > > > > something that has the semantics of "we will now ignore preload=none, > > clamping > > > > the effective preload state to metadata or higher depending on other > state". > > > Not > > > > sure what to call it, maybe you can see if it would actually work first? > > > > > > ok, i have tried the changes locally and all the layout tests are passing. > But > > > one problem here is the flag m_havePreparedToPlay is currently being reset > in > > > prepareForLoad(), so right now if i set the flag inbetween prepareforload > and > > > loadinternal it is working fine. But once we remove all this infavor of > > > invokeLoadAlgorithm, how can we handle this flag? Should i reset the flag at > > the > > > call sites ? > > > > Let's see, m_havePreparedToPlay is used in two ways: > > > > 1. It's combined with effectivePreloadType() to possibly defer the load in > > HTMLMediaElement::loadResource, which is very much related to > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921 > > > > 2. It can cause an early return in HTMLMediaElement::prepareToPlay(), and > > removing that could cause startDeferredLoad() to be called twice if > > loadIsDeferred(), but judging by the various methods involved in load > deferral, > > it looks like perhaps this isn't observable at all. > > > > So, I would propose removing prepareToPlay() and m_havePreparedToPlay and > > replacing them with two things that explicitly do what seems to be the > important > > bits: > > > > 1. A flag which causes effectivePreloadType() to return PreloadAuto if it is > > set, and it is set by playing and seeking. > > > > 2. Whatever changes, if any, are required to ensure that a deferred load is > > started precisely when it should be, at least when seeking and playing. May > > require some changes to the whole deferred load machinery. > > I will work on these suggestions Regarding the first point: "1. A flag which causes effectivePreloadType() to return PreloadAuto if it is set, and it is set by playing and seeking." As per the spec bug, When the flag is set, We should return PreloadMetadata if the preload value is PreloadNone, or did i understand it in wrong way?
On 2016/02/16 14:10:57, Srirama wrote: > Regarding the first point: > "1. A flag which causes effectivePreloadType() to return PreloadAuto if it is > set, and it is set by playing and seeking." > As per the spec bug, When the flag is set, We should return PreloadMetadata if > the preload value is PreloadNone, or did i understand it in wrong way? Yes, what is described in https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921#c3 is a flag that is set when load() is called, which if set causes the effective preload state to be no less than "metadata", i.e. if it is "none" it will be "preload" instead.
Any progress on this? In https://bugs.chromium.org/p/chromium/issues/detail?id=587789#c19 it looks like our bugs here is the source of observably different behavior in the case of captureStream() from canvas. I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=592396 and will add that to the issue description.
Description was changed from ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. ========== to ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. BUG=592396 ==========
On 2016/03/07 11:14:55, philipj_UTC7 wrote: > Any progress on this? In > https://bugs.chromium.org/p/chromium/issues/detail?id=587789#c19 it looks like > our bugs here is the source of observably different behavior in the case of > captureStream() from canvas. I've filed > https://bugs.chromium.org/p/chromium/issues/detail?id=592396 and will add that > to the issue description. Sorry, i was busy with my internal work, so couldn't get time to look into it. I have almost done the changes which we have discussed above except the "diferred load machinery" changes. Probably i will upload the changes today if possible or tomorrow.
On 2016/03/07 11:26:48, Srirama wrote: > On 2016/03/07 11:14:55, philipj_UTC7 wrote: > > Any progress on this? In > > https://bugs.chromium.org/p/chromium/issues/detail?id=587789#c19 it looks like > > our bugs here is the source of observably different behavior in the case of > > captureStream() from canvas. I've filed > > https://bugs.chromium.org/p/chromium/issues/detail?id=592396 and will add that > > to the issue description. > > Sorry, i was busy with my internal work, so couldn't get time to look into it. I > have almost done the changes which we have discussed above except the "diferred > load machinery" changes. > Probably i will upload the changes today if possible or tomorrow. PTAL. Below are the 2 action items given by you. 1. A flag which causes effectivePreloadType() to return PreloadAuto if it is set, and it is set by playing and seeking. I have done the changes except setting the flag in playing and seeking. I will do that change in my next CL. 2. Whatever changes, if any, are required to ensure that a deferred load is started precisely when it should be, at least when seeking and playing. May require some changes to the whole deferred load machinery. Do you want me to do these changes as part of this CL or is it ok if i tackle it in a separate CL?
On 2016/03/08 11:50:56, Srirama wrote: > On 2016/03/07 11:26:48, Srirama wrote: > > On 2016/03/07 11:14:55, philipj_UTC7 wrote: > > > Any progress on this? In > > > https://bugs.chromium.org/p/chromium/issues/detail?id=587789#c19 it looks > like > > > our bugs here is the source of observably different behavior in the case of > > > captureStream() from canvas. I've filed > > > https://bugs.chromium.org/p/chromium/issues/detail?id=592396 and will add > that > > > to the issue description. > > > > Sorry, i was busy with my internal work, so couldn't get time to look into it. > I > > have almost done the changes which we have discussed above except the > "diferred > > load machinery" changes. > > Probably i will upload the changes today if possible or tomorrow. > > PTAL. Below are the 2 action items given by you. > 1. A flag which causes effectivePreloadType() to return PreloadAuto if it is > set, and it is set by playing and seeking. > I have done the changes except setting the flag in playing and seeking. I will > do that change in my next CL. Do you mean next CL or next PS? > 2. Whatever changes, if any, are required to ensure that a deferred load is > started precisely when it should be, at least when seeking and playing. May > require some changes to the whole deferred load machinery. > Do you want me to do these changes as part of this CL or is it ok if i tackle it > in a separate CL? The CL in its current form is a mix of the old and new models and I'm not confident that I even understand how it will work. I think I'd prefer sorting this out in one go, unless there are logical smaller steps. Not sure what such a small step would be, though. Will comment on the bits that still don't look quite right.
On 2016/03/09 09:12:00, philipj_UTC7 wrote: > On 2016/03/08 11:50:56, Srirama wrote: > > On 2016/03/07 11:26:48, Srirama wrote: > > > On 2016/03/07 11:14:55, philipj_UTC7 wrote: > > > > Any progress on this? In > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=587789#c19 it looks > > like > > > > our bugs here is the source of observably different behavior in the case > of > > > > captureStream() from canvas. I've filed > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=592396 and will add > > that > > > > to the issue description. > > > > > > Sorry, i was busy with my internal work, so couldn't get time to look into > it. > > I > > > have almost done the changes which we have discussed above except the > > "diferred > > > load machinery" changes. > > > Probably i will upload the changes today if possible or tomorrow. > > > > PTAL. Below are the 2 action items given by you. > > 1. A flag which causes effectivePreloadType() to return PreloadAuto if it is > > set, and it is set by playing and seeking. > > I have done the changes except setting the flag in playing and seeking. I will > > do that change in my next CL. > > Do you mean next CL or next PS? Sorry, i mean next PS along with fixes for review comments from you for the current PS. > > > 2. Whatever changes, if any, are required to ensure that a deferred load is > > started precisely when it should be, at least when seeking and playing. May > > require some changes to the whole deferred load machinery. > > Do you want me to do these changes as part of this CL or is it ok if i tackle > it > > in a separate CL? > > The CL in its current form is a mix of the old and new models and I'm not > confident that I even understand how it will work. I think I'd prefer sorting > this out in one go, unless there are logical smaller steps. Not sure what such a > small step would be, though. > > Will comment on the bits that still don't look quite right. Ok, you are right, i will finish it off in this CL itself. Please elaborate a bit on how exactly you want the "defered load machinery" to be changed".
https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:706: prepareForLoad(); There ought to be a call to invokeLoadAlgorithm() here and not prepareForLoad() + loadInternal(). https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2021: scheduleNextSourceChild(); Remove scheduleNextSourceChild(), if it's breaks then there are more things in need of fixing :/ https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2086: scheduleNextSourceChild(); Ditto https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2717: scheduleNextSourceChild(); Ditto. It would also be *nice* if m_nextChildNodeToConsider were maintained outside of this if clause, and I'm not sure it's correct. However, that all looks pretty involved and it's probably best to not refactor it here. If you understand why the code is like this a comment would be nice, otherwise just leave it as is. https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:556: bool m_ignorePreloadNone : 1; m_forceEffectivePreloadAuto might be a more descriptive name
https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1617: startDeferredLoad(); OK, so the removal is this code is the "deferred load machinery". The changes you've made look like they could work. However, it looks like you could actually reintroduce something very much like prepareToPlay that both sets m_forceEffectivePreloadAuto and then calls setPlayerPreload() which will also start the load if necessary. https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:706: prepareForLoad(); On 2016/03/09 09:15:59, philipj_UTC7 wrote: > There ought to be a call to invokeLoadAlgorithm() here and not prepareForLoad() > + loadInternal(). But I suppose you will need if (loadIsDeferred()) startDeferredLoad() here.
https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:706: prepareForLoad(); On 2016/03/09 09:27:23, philipj_UTC7 wrote: > On 2016/03/09 09:15:59, philipj_UTC7 wrote: > > There ought to be a call to invokeLoadAlgorithm() here and not > prepareForLoad() > > + loadInternal(). > > But I suppose you will need if (loadIsDeferred()) startDeferredLoad() here. Sorry, I'm writing comments out of order. This could presumably become simply invokeLoadAlgorithm() followed by a call to the function that sets m_forceEffectivePreloadAuto and calls setPlayerPreload(). Since invokeLoadAlgorithm() will already have destroyed the player it would effectively be the same as just setting the flag.
PTAL https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1617: startDeferredLoad(); On 2016/03/09 09:27:23, philipj_UTC7 wrote: > OK, so the removal is this code is the "deferred load machinery". The changes > you've made look like they could work. However, it looks like you could actually > reintroduce something very much like prepareToPlay that both sets > m_forceEffectivePreloadAuto and then calls setPlayerPreload() which will also > start the load if necessary. Added prepareToPlay again and modified as suggested. Do you want me to change the name? https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:706: prepareForLoad(); On 2016/03/09 09:30:22, philipj_UTC7 wrote: > On 2016/03/09 09:27:23, philipj_UTC7 wrote: > > On 2016/03/09 09:15:59, philipj_UTC7 wrote: > > > There ought to be a call to invokeLoadAlgorithm() here and not > > prepareForLoad() > > > + loadInternal(). > > > > But I suppose you will need if (loadIsDeferred()) startDeferredLoad() here. > > Sorry, I'm writing comments out of order. This could presumably become simply > invokeLoadAlgorithm() followed by a call to the function that sets > m_forceEffectivePreloadAuto and calls setPlayerPreload(). Since > invokeLoadAlgorithm() will already have destroyed the player it would > effectively be the same as just setting the flag. Replaced prepareForLoad + loadInternal with invokeLoadAlgorithm(). But do we need to call one more function here? InvokeLoadAlgorithm internally schedules loadInternal and we need to set the flag m_forceEffectivePreloadAuto before reaching loadInternal. So i am setting the flag before calling invokeLoadAlgorithm(). https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2021: scheduleNextSourceChild(); On 2016/03/09 09:16:00, philipj_UTC7 wrote: > Remove scheduleNextSourceChild(), if it's breaks then there are more things in > need of fixing :/ Right now invokeResourceSelectionAlgorithm doesn't take care of 4th step. 4th step is taken care in scheduleNextSourceChild(). So keeping it for now with a TODO mentioning the bug you raised. https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2086: scheduleNextSourceChild(); On 2016/03/09 09:16:00, philipj_UTC7 wrote: > Ditto See my comment above. https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2717: scheduleNextSourceChild(); On 2016/03/09 09:16:00, philipj_UTC7 wrote: > Ditto. It would also be *nice* if m_nextChildNodeToConsider were maintained > outside of this if clause, and I'm not sure it's correct. However, that all > looks pretty involved and it's probably best to not refactor it here. If you > understand why the code is like this a comment would be nice, otherwise just > leave it as is. I think if we move m_nextChildNodeToConsider above there will be behaviour change as there is a chance of never reaching the steps 2731 onwards. I think the idea is to consider new |source| only if the current |m_nextChildNodeToConsider| processing has not yet started. https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:556: bool m_ignorePreloadNone : 1; On 2016/03/09 09:16:00, philipj_UTC7 wrote: > m_forceEffectivePreloadAuto might be a more descriptive name Done.
On 2016/03/09 12:26:22, Srirama wrote: > PTAL > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1617: > startDeferredLoad(); > On 2016/03/09 09:27:23, philipj_UTC7 wrote: > > OK, so the removal is this code is the "deferred load machinery". The changes > > you've made look like they could work. However, it looks like you could > actually > > reintroduce something very much like prepareToPlay that both sets > > m_forceEffectivePreloadAuto and then calls setPlayerPreload() which will also > > start the load if necessary. > > Added prepareToPlay again and modified as suggested. Do you want me to change > the name? > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:706: prepareForLoad(); > On 2016/03/09 09:30:22, philipj_UTC7 wrote: > > On 2016/03/09 09:27:23, philipj_UTC7 wrote: > > > On 2016/03/09 09:15:59, philipj_UTC7 wrote: > > > > There ought to be a call to invokeLoadAlgorithm() here and not > > > prepareForLoad() > > > > + loadInternal(). > > > > > > But I suppose you will need if (loadIsDeferred()) startDeferredLoad() here. > > > > Sorry, I'm writing comments out of order. This could presumably become simply > > invokeLoadAlgorithm() followed by a call to the function that sets > > m_forceEffectivePreloadAuto and calls setPlayerPreload(). Since > > invokeLoadAlgorithm() will already have destroyed the player it would > > effectively be the same as just setting the flag. > > Replaced prepareForLoad + loadInternal with invokeLoadAlgorithm(). But do we > need to call one more function here? InvokeLoadAlgorithm internally schedules > loadInternal and we need to set the flag m_forceEffectivePreloadAuto before > reaching loadInternal. So i am setting the flag before calling > invokeLoadAlgorithm(). > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2021: > scheduleNextSourceChild(); > On 2016/03/09 09:16:00, philipj_UTC7 wrote: > > Remove scheduleNextSourceChild(), if it's breaks then there are more things in > > need of fixing :/ > > Right now invokeResourceSelectionAlgorithm doesn't take care of 4th step. 4th > step is taken care in scheduleNextSourceChild(). So keeping it for now with a > TODO mentioning the bug you raised. > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2086: > scheduleNextSourceChild(); > On 2016/03/09 09:16:00, philipj_UTC7 wrote: > > Ditto > > See my comment above. > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2717: > scheduleNextSourceChild(); > On 2016/03/09 09:16:00, philipj_UTC7 wrote: > > Ditto. It would also be *nice* if m_nextChildNodeToConsider were maintained > > outside of this if clause, and I'm not sure it's correct. However, that all > > looks pretty involved and it's probably best to not refactor it here. If you > > understand why the code is like this a comment would be nice, otherwise just > > leave it as is. > > I think if we move m_nextChildNodeToConsider above there will be behaviour > change as there is a chance of never reaching the steps 2731 onwards. I think > the idea is to consider new |source| only if the current > |m_nextChildNodeToConsider| processing has not yet started. > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): > > https://codereview.chromium.org/1522463003/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.h:556: bool > m_ignorePreloadNone : 1; > On 2016/03/09 09:16:00, philipj_UTC7 wrote: > > m_forceEffectivePreloadAuto might be a more descriptive name > > Done. Ping. Please have a look. I will start "microtask" changes once this lands.
OK, code changes now look good, but some test coverage is needed. If possible please use testharness.js. See comment about Internals.idl if there doesn't seem to be any other way of testing the changes. https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:731: m_havePreparedToPlay = false; Actually, the renamed state bit should also be reset here, otherwise it'll be impossible to reuse a media element for a new load with preload=auto. This could be a problem in the current shape of load() because the `m_forceEffectivePreloadAuto = true` before invokeLoadAlgorithm() is immediately undone, but setting it after is too late. Once the resource selection algorithm is async, however, the order shouldn't matter. If necessary to get things working in the interim, maybe you can do `m_forceEffectivePreloadAuto = false` before call to invokeLoadAlgorithm() except the one in load(), with TODOs to move it into prepareForLoad() together with the microtask fix? https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1615: void HTMLMediaElement::prepareToPlay() This isn't only in preparation to play, but what would be a good name? If you can't think of one, maybe just setIgnorePreloadNone()? https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1952: if (m_forceEffectivePreloadAuto && preload == WebMediaPlayer::PreloadNone) Looking at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921#c3 again, I see that this isn't quite what Firefox does. If this instead returns WebMediaPlayer::PreloadMetaData to match Firefox, a very explicit name would be m_clampEffectivePreloadToMetadataOrHigher, but maybe m_ignorePreloadNone would be an OK name. As for the difference, if the distinction is actually made in WebMediaPlayerImpl, it would be in the amount of data that is buffered after seeking. Either as little as possible (metadata) or as much as possible (auto). Might be worth testing manually, but it's a hard to write a fast and reliable test for this kind of thing. Maybe att effectivePreload to Internals.idl to test this?
Oh, it looks like clearMediaPlayer() is now only called from stop(), which if the final state would mean that its argument can be removed. It looks like you should be getting closer to simplifying the resetMediaPlayerAndMediaSource() vs. clearMediaPlayer() mess this started with too, but not in the CL obviously. Since prepareForLoad() is now only called from invokeLoadAlgorithm(), it can be made part of that, it doesn't correspond to any specific concept from the spec. I ran some tests locally and imported/web-platform-tests/html/semantics/embedded-content/media-elements/ need updated expectations, but you'll notice that when you run the bots.
This CL has lot of refactoring without much changes in behavior. So do you want the test coverage for load function changes? https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:731: m_havePreparedToPlay = false; On 2016/03/11 12:52:41, philipj_UTC7 wrote: > Actually, the renamed state bit should also be reset here, otherwise it'll be > impossible to reuse a media element for a new load with preload=auto. This > could be a problem in the current shape of load() because the > `m_forceEffectivePreloadAuto = true` before invokeLoadAlgorithm() is immediately > undone, but setting it after is too late. Once the resource selection algorithm > is async, however, the order shouldn't matter. > > If necessary to get things working in the interim, maybe you can do > `m_forceEffectivePreloadAuto = false` before call to invokeLoadAlgorithm() > except the one in load(), with TODOs to move it into prepareForLoad() together > with the microtask fix? Done. https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1615: void HTMLMediaElement::prepareToPlay() On 2016/03/11 12:52:41, philipj_UTC7 wrote: > This isn't only in preparation to play, but what would be a good name? If you > can't think of one, maybe just setIgnorePreloadNone()? I will go with setIgnorePreloadNone. https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1952: if (m_forceEffectivePreloadAuto && preload == WebMediaPlayer::PreloadNone) On 2016/03/11 12:52:41, philipj_UTC7 wrote: > Looking at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921#c3 again, I see > that this isn't quite what Firefox does. > > If this instead returns WebMediaPlayer::PreloadMetaData to match Firefox, a very > explicit name would be m_clampEffectivePreloadToMetadataOrHigher, but maybe > m_ignorePreloadNone would be an OK name. > > As for the difference, if the distinction is actually made in > WebMediaPlayerImpl, it would be in the amount of data that is buffered after > seeking. Either as little as possible (metadata) or as much as possible (auto). > Might be worth testing manually, but it's a hard to write a fast and reliable > test for this kind of thing. Maybe att effectivePreload to Internals.idl to test > this? You mean *"Maybe add effectivePreload"? I will try adding it to internals.idl and write a test.
https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1952: if (m_forceEffectivePreloadAuto && preload == WebMediaPlayer::PreloadNone) On 2016/03/11 14:58:20, Srirama wrote: > On 2016/03/11 12:52:41, philipj_UTC7 wrote: > > Looking at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921#c3 again, I > see > > that this isn't quite what Firefox does. > > > > If this instead returns WebMediaPlayer::PreloadMetaData to match Firefox, a > very > > explicit name would be m_clampEffectivePreloadToMetadataOrHigher, but maybe > > m_ignorePreloadNone would be an OK name. > > > > As for the difference, if the distinction is actually made in > > WebMediaPlayerImpl, it would be in the amount of data that is buffered after > > seeking. Either as little as possible (metadata) or as much as possible > (auto). > > Might be worth testing manually, but it's a hard to write a fast and reliable > > test for this kind of thing. Maybe att effectivePreload to Internals.idl to > test > > this? > > You mean *"Maybe add effectivePreload"? I will try adding it to internals.idl > and write a test. Yes, right. The tests could simply verify that load(), play() and setting currentTime forces the effective preload state to metadata if it was none.
Patchset #12 (id:300001) has been deleted
On 2016/03/11 18:57:38, philipj_UTC7 wrote: > https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1522463003/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1952: if > (m_forceEffectivePreloadAuto && preload == WebMediaPlayer::PreloadNone) > On 2016/03/11 14:58:20, Srirama wrote: > > On 2016/03/11 12:52:41, philipj_UTC7 wrote: > > > Looking at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28921#c3 again, I > > see > > > that this isn't quite what Firefox does. > > > > > > If this instead returns WebMediaPlayer::PreloadMetaData to match Firefox, a > > very > > > explicit name would be m_clampEffectivePreloadToMetadataOrHigher, but maybe > > > m_ignorePreloadNone would be an OK name. > > > > > > As for the difference, if the distinction is actually made in > > > WebMediaPlayerImpl, it would be in the amount of data that is buffered after > > > seeking. Either as little as possible (metadata) or as much as possible > > (auto). > > > Might be worth testing manually, but it's a hard to write a fast and > reliable > > > test for this kind of thing. Maybe att effectivePreload to Internals.idl to > > test > > > this? > > > > You mean *"Maybe add effectivePreload"? I will try adding it to internals.idl > > and write a test. > > Yes, right. The tests could simply verify that load(), play() and setting > currentTime forces the effective preload state to metadata if it was none. Added test cases for load and play scenarios but not able to add for seek scenario, as the seek function early returns when the ready state is HAVE_NOTHING. PTAL.
lgtm % nits https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-creation.html (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-creation.html:22: video.load(); The extra load() after setting src should now be redundant, remove? https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-force-preload-none-to-metadata-on-load.html (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-force-preload-none-to-metadata-on-load.html:8: async_test(function(t) { Since this doesn't have any async part, it can use just test(), which removes the t.done() line. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:544: if (!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) { Just want to say that this was pretty strange to begin with and is still pretty strange. What the spec once said was to start the resource selection algorithm here. But I don't suggest that, because these conditions somewhat limit the cases when this now non-standard behavior comes into play, and that's good. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:796: scheduleNextSourceChild(); Oh but wait, every call to invokeResourceSelectionAlgorithm() is now followed by scheduleNextSourceChild(), so you could move that into invokeResourceSelectionAlgorithm() and move the TODOs about the next step into there. But also OK to leave it as is if you like, since you'll nuke it all in the next CL anyway. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1633: setIgnorePreloadNone(); This actually looks like it could unconditionally call setIgnorePreloadNone() now, that's closer to how I imagine this will be spec'd and looks like it will do the right thing without any further changes. Comment just needs adjusting somehow. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3080: setIgnorePreloadNone(); I looked at more context and realize I don't understand why this works, or worked before. This is inside the branch for pausing, so it can't be the reason why the play() test passes? Can you try to move (an unconditional) setIgnorePreloadNone() to play() and see if this can just be removed? https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/Internals.cpp:1873: return static_cast<unsigned short>(mediaElement->effectivePreloadType()); Can you call if just effectivePreload and return a string in a fashion similar to HTMLMediaElement::preload()? It could perhaps be moved into HTMLMediaElement itself at a later point if a effectivePreload attribute is added to HTMLMediaElement, which think is worth considering at least.
PTAL. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-creation.html (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/HTMLMediaElementCapture-creation.html:22: video.load(); On 2016/03/14 13:03:15, philipj_UTC7 wrote: > The extra load() after setting src should now be redundant, remove? Done. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-force-preload-none-to-metadata-on-load.html (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-force-preload-none-to-metadata-on-load.html:8: async_test(function(t) { On 2016/03/14 13:03:15, philipj_UTC7 wrote: > Since this doesn't have any async part, it can use just test(), which removes > the t.done() line. Done. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:544: if (!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) { On 2016/03/14 13:03:15, philipj_UTC7 wrote: > Just want to say that this was pretty strange to begin with and is still pretty > strange. What the spec once said was to start the resource selection algorithm > here. But I don't suggest that, because these conditions somewhat limit the > cases when this now non-standard behavior comes into play, and that's good. Acknowledged. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:796: scheduleNextSourceChild(); On 2016/03/14 13:03:15, philipj_UTC7 wrote: > Oh but wait, every call to invokeResourceSelectionAlgorithm() is now followed by > scheduleNextSourceChild(), so you could move that into > invokeResourceSelectionAlgorithm() and move the TODOs about the next step into > there. But also OK to leave it as is if you like, since you'll nuke it all in > the next CL anyway. Done. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1633: setIgnorePreloadNone(); On 2016/03/14 13:03:15, philipj_UTC7 wrote: > This actually looks like it could unconditionally call setIgnorePreloadNone() > now, that's closer to how I imagine this will be spec'd and looks like it will > do the right thing without any further changes. Comment just needs adjusting > somehow. Done. I thought existing comment is ok. Anyway i have changed it a bit, not sure if it is good enough :) https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3080: setIgnorePreloadNone(); On 2016/03/14 13:03:15, philipj_UTC7 wrote: > I looked at more context and realize I don't understand why this works, or > worked before. This is inside the branch for pausing, so it can't be the reason > why the play() test passes? > > Can you try to move (an unconditional) setIgnorePreloadNone() to play() and see > if this can just be removed? Infact i had the same doubt initially when you said we need to set the flag incase of play() and I confirmed with logs that this actually is coming incase of play(). Didn't debug deeper though how it is coming here. Anyway i have moved in inside playinternal() just before invoking updatePlayState(). https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/Internals.cpp:1873: return static_cast<unsigned short>(mediaElement->effectivePreloadType()); On 2016/03/14 13:03:15, philipj_UTC7 wrote: > Can you call if just effectivePreload and return a string in a fashion similar > to HTMLMediaElement::preload()? It could perhaps be moved into HTMLMediaElement > itself at a later point if a effectivePreload attribute is added to > HTMLMediaElement, which think is worth considering at least. Done.
Still lgtm % nits. Feel free to CQ without further review when addressed. https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1633: setIgnorePreloadNone(); On 2016/03/14 15:12:54, Srirama wrote: > On 2016/03/14 13:03:15, philipj_UTC7 wrote: > > This actually looks like it could unconditionally call setIgnorePreloadNone() > > now, that's closer to how I imagine this will be spec'd and looks like it will > > do the right thing without any further changes. Comment just needs adjusting > > somehow. > > Done. > I thought existing comment is ok. Anyway i have changed it a bit, not sure if it > is good enough :) Looks great! https://codereview.chromium.org/1522463003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3080: setIgnorePreloadNone(); On 2016/03/14 15:12:54, Srirama wrote: > On 2016/03/14 13:03:15, philipj_UTC7 wrote: > > I looked at more context and realize I don't understand why this works, or > > worked before. This is inside the branch for pausing, so it can't be the > reason > > why the play() test passes? > > > > Can you try to move (an unconditional) setIgnorePreloadNone() to play() and > see > > if this can just be removed? > > Infact i had the same doubt initially when you said we need to set the flag > incase of play() and I confirmed with logs that this actually is coming incase > of play(). Didn't debug deeper though how it is coming here. > Anyway i have moved in inside playinternal() just before invoking > updatePlayState(). OK, that's weird, makes more sense when moved, thanks! https://codereview.chromium.org/1522463003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1947: String HTMLMediaElement::effectivePreload() const Oh, yeah, putting this in HTMLMediaElement right away makes sense. Can you break out the enum-to-string conversion to a preloadTypeToString that's shared with preload()?
rs LGTM
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1522463003/#ps360001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522463003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522463003/360001
https://codereview.chromium.org/1522463003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1522463003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.h:115: String preloadTypeToString(WebMediaPlayer::Preload preloadType) const; This ought to be a helper in the cpp file only, it doesn't need to be a member. See AudioKindToString and similar.
The CQ bit was unchecked by srirama.m@samsung.com
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1522463003/#ps380001 (title: "Make preloadTypeToString a helper function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522463003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522463003/380001
The CQ bit was unchecked by philipj@opera.com
On 2016/03/15 05:13:28, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1522463003/380001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1522463003/380001 Sorry, did i miss something else?
@philip, does it need some more changes before sending it to CQ? I have made preloadTypeToString() as a helper function. I'm afraid, did i miss anything?
https://codereview.chromium.org/1522463003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1522463003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1891: const String preloadTypeToString(WebMediaPlayer::Preload preloadType) Helpers like this should be in anonymous namespace at the top. The const keyword isn't helping here, I think it'll only cause an extra copy in order to get from "const String" to "String" at the call site.
On 2016/03/15 07:30:18, Srirama wrote: > @philip, does it need some more changes before sending it to CQ? I have made > preloadTypeToString() as a helper function. I'm afraid, did i miss anything? Sorry, I didn't realize the latest PS had the changes when I unchecked CQ. One more style nit.
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1522463003/#ps400001 (title: "Move preloadTypeToString to anonymous namespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522463003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522463003/400001
philipj@opera.com changed reviewers: + mlamouri@chromium.org
mlamouri@ was interested in this.
Message was sent while issue was closed.
Description was changed from ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. BUG=592396 ========== to ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. BUG=592396 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. BUG=592396 ========== to ========== Refactor resource load and resource selection algorithms as per spec In the current code the resource load and resource selection algorithms were mixed and there are no explicit entry points. Added new functions invokeLoadAlgorithm and invokeResourceSelectionAlgorithm to separate them and partially updated the code to match the spec. BUG=592396 Committed: https://crrev.com/4563c7c2c70ab5f09a017cad834d995485f75ec0 Cr-Commit-Position: refs/heads/master@{#381203} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/4563c7c2c70ab5f09a017cad834d995485f75ec0 Cr-Commit-Position: refs/heads/master@{#381203} |
