Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1810513002:
Media element resource selection algorithm should "await a stable state"
|
Created:
4 years, 9 months ago by Srirama Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia element resource selection algorithm should "await a stable state"
Right now in HTMLMediaElement we post a plain task for
"await a stable state" step in resource selection algorithm. But as per
the spec we should post a microtask. Implemented microtask for the same.
BUG=593289
Patch Set 1 #
Total comments: 9
Patch Set 2 : changed loadtimer to texttrackloadtimer #
Total comments: 21
Patch Set 3 : Use CancellableTaskFactory #
Total comments: 7
Patch Set 4 : Rebase and address review comments #
Total comments: 2
Patch Set 5 : Rebase and comment fix #Patch Set 6 : Rebase and microtask changes #
Total comments: 1
Patch Set 7 : Rebase, microtask changes, layouttest updation #
Messages
Total messages: 91 (19 generated)
Description was changed from ========== fix fix fix microtask changes BUG= ========== to ========== Media element resource selection algorithm should "await a stable state" Right now in HTMLMediaElement we post a plain task for "await a stable state" in resource selection algorithm. But as per the spec we should post a microtask. Implemented microtask for the same. BUG=593289 ==========
Description was changed from ========== Media element resource selection algorithm should "await a stable state" Right now in HTMLMediaElement we post a plain task for "await a stable state" in resource selection algorithm. But as per the spec we should post a microtask. Implemented microtask for the same. BUG=593289 ========== to ========== Media element resource selection algorithm should "await a stable state" Right now in HTMLMediaElement we post a plain task for "await a stable state" step in resource selection algorithm. But as per the spec we should post a microtask. Implemented microtask for the same. BUG=593289 ==========
srirama.m@samsung.com changed reviewers: + philipj@opera.com
PTAL. If these changes are ok, then i will update the test expectations for web-platform-tests.
https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: Microtask::enqueueMicrotask(task.release()); If you have a look at RTCPeerConnection.cpp and HTMLSlotElement.cpp, there's a way to enqueue a microtask that keeps the media element alive with a one-liner. I learned about this in https://codereview.chromium.org/1712333002 and think it should work here too. https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:900: void HTMLMediaElement::continueResourceSelectionAlgorithm() All of this method looks a bit strange, should it not just consist of steps 5 and on in https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... ? At least the text track bits shouldn't be here I think, and I think it ought to be possible to get rid of the LoadMediaResource flag and only depend on the new things you've added soon? If not, what remains before this can all be a 1:1 mapping of the spec?
https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:900: void HTMLMediaElement::continueResourceSelectionAlgorithm() On 2016/03/16 13:40:20, philipj_UTC7 wrote: > All of this method looks a bit strange, should it not just consist of steps 5 > and on in > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > ? > > At least the text track bits shouldn't be here I think, and I think it ought to > be possible to get rid of the LoadMediaResource flag and only depend on the new > things you've added soon? If not, what remains before this can all be a 1:1 > mapping of the spec? Without these changes all the media/track test cases are failing. Right now text track uses loadtimer task and now we added micro task for resource selection. So should we keep them seperate? May be microtask is fired faster than the loadtimer task and probably that is the reason why all those tests are failing. I will debug further.
https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:900: void HTMLMediaElement::continueResourceSelectionAlgorithm() On 2016/03/16 14:25:40, Srirama wrote: > On 2016/03/16 13:40:20, philipj_UTC7 wrote: > > All of this method looks a bit strange, should it not just consist of steps 5 > > and on in > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > ? > > > > At least the text track bits shouldn't be here I think, and I think it ought > to > > be possible to get rid of the LoadMediaResource flag and only depend on the > new > > things you've added soon? If not, what remains before this can all be a 1:1 > > mapping of the spec? > > Without these changes all the media/track test cases are failing. Right now text > track uses loadtimer task and now we added micro task for resource selection. So > should we keep them seperate? May be microtask is fired faster than the > loadtimer task and probably that is the reason why all those tests are failing. > I will debug further. loadtimer started by scheduleTextTrackResourceLoad is being stopped in invokeloadalgorithm and now we are posting microtask. Previously we used to post loadtimer and that used to take care of texttrack as well.
https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: Microtask::enqueueMicrotask(task.release()); On 2016/03/16 13:40:20, philipj_UTC7 wrote: > If you have a look at RTCPeerConnection.cpp and HTMLSlotElement.cpp, there's a > way to enqueue a microtask that keeps the media element alive with a one-liner. > I learned about this in https://codereview.chromium.org/1712333002 and think it > should work here too. OK, so one problem could be where the spec says "Abort any already-running instance of the resource selection algorithm for this element" so it'll actually be necessary to cancel a pending microtask. Is this the only reason that you ended up with a separate Task class, or is there more to it? https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:900: void HTMLMediaElement::continueResourceSelectionAlgorithm() On 2016/03/16 14:35:23, Srirama wrote: > On 2016/03/16 14:25:40, Srirama wrote: > > On 2016/03/16 13:40:20, philipj_UTC7 wrote: > > > All of this method looks a bit strange, should it not just consist of steps > 5 > > > and on in > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > ? > > > > > > At least the text track bits shouldn't be here I think, and I think it ought > > to > > > be possible to get rid of the LoadMediaResource flag and only depend on the > > new > > > things you've added soon? If not, what remains before this can all be a 1:1 > > > mapping of the spec? > > > > Without these changes all the media/track test cases are failing. Right now > text > > track uses loadtimer task and now we added micro task for resource selection. > So > > should we keep them seperate? May be microtask is fired faster than the > > loadtimer task and probably that is the reason why all those tests are > failing. > > I will debug further. > > loadtimer started by scheduleTextTrackResourceLoad is being stopped in > invokeloadalgorithm and now we are posting microtask. Previously we used to post > loadtimer and that used to take care of texttrack as well. I think that really honorUserPreferencesForAutomaticTextTrackSelection() should simply be called in the two places where the spec says so, if you follow the back references in https://html.spec.whatwg.org/multipage/embedded-content.html#honor-user-prefe... If that still breaks some tests, I guess the current implementation isn't per spec and the difference matters, in which case we'll have to look at precisely what's wrong.
https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: Microtask::enqueueMicrotask(task.release()); Infact when i started i just copied it from ImageLoader but i realized that we need it for aborting the already running instance. This is the only reason. On 2016/03/16 15:35:10, philipj_UTC7 wrote: > On 2016/03/16 13:40:20, philipj_UTC7 wrote: > > If you have a look at RTCPeerConnection.cpp and HTMLSlotElement.cpp, there's a > > way to enqueue a microtask that keeps the media element alive with a > one-liner. > > I learned about this in https://codereview.chromium.org/1712333002 and think > it > > should work here too. > > OK, so one problem could be where the spec says "Abort any already-running > instance of the resource selection algorithm for this element" so it'll actually > be necessary to cancel a pending microtask. Is this the only reason that you > ended up with a separate Task class, or is there more to it? https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:900: void HTMLMediaElement::continueResourceSelectionAlgorithm() Even i was going through it and wondering why it was held back till load timer. I think this will work. On 2016/03/16 15:35:10, philipj_UTC7 wrote: > On 2016/03/16 14:35:23, Srirama wrote: > > On 2016/03/16 14:25:40, Srirama wrote: > > > On 2016/03/16 13:40:20, philipj_UTC7 wrote: > > > > All of this method looks a bit strange, should it not just consist of > steps > > 5 > > > > and on in > > > > > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > > ? > > > > > > > > At least the text track bits shouldn't be here I think, and I think it > ought > > > to > > > > be possible to get rid of the LoadMediaResource flag and only depend on > the > > > new > > > > things you've added soon? If not, what remains before this can all be a > 1:1 > > > > mapping of the spec? > > > > > > Without these changes all the media/track test cases are failing. Right now > > text > > > track uses loadtimer task and now we added micro task for resource > selection. > > So > > > should we keep them seperate? May be microtask is fired faster than the > > > loadtimer task and probably that is the reason why all those tests are > > failing. > > > I will debug further. > > > > loadtimer started by scheduleTextTrackResourceLoad is being stopped in > > invokeloadalgorithm and now we are posting microtask. Previously we used to > post > > loadtimer and that used to take care of texttrack as well. > > I think that really honorUserPreferencesForAutomaticTextTrackSelection() should > simply be called in the two places where the spec says so, if you follow the > back references in > https://html.spec.whatwg.org/multipage/embedded-content.html#honor-user-prefe... > > If that still breaks some tests, I guess the current implementation isn't per > spec and the difference matters, in which case we'll have to look at precisely > what's wrong.
I have removed scheduleTextTrackResurceLoad and directly called honorUser..... function. All the tests are passing except track-language-preference.html. Any quick pointer here why it is failing. Expected: *********************************** **Set user preferred languages RUN(internals.setUserPreferredLanguages(['jp', 'es-ES', 'en', 'fr'])) Test: a track language matches one of the user's preferred languages exactly. - creating tracks for: [fr,en,jp]. EVENT(load) EXPECTED (track.readyState == '2') OK EXPECTED (track.srclang == 'jp') OK ********************************** Actual: ***************************************** **Set user preferred languages RUN(internals.setUserPreferredLanguages(['jp', 'es-ES', 'en', 'fr'])) Test: a track language matches one of the user's preferred languages exactly. - creating tracks for: [fr,en,jp]. EVENT(load) EXPECTED (track.readyState == '2') OK EXPECTED (track.srclang == 'jp'), OBSERVED 'fr' FAIL EVENT(load) EXPECTED (track.readyState == '2') OK EXPECTED (track.srclang == 'jp'), OBSERVED 'en' FAIL EVENT(load) EXPECTED (track.readyState == '2') OK EXPECTED (track.srclang == 'jp') OK ********************************************
On 2016/03/17 at 07:53:08, srirama.m wrote: > I have removed scheduleTextTrackResurceLoad and directly called honorUser..... function. All the tests are passing except track-language-preference.html. > Any quick pointer here why it is failing. A guess would be that the scheduleDelayedAction in didAddTrackElement now no longer trigger honorUser... as before.
On 2016/03/17 09:49:35, fs (traveling) wrote: > On 2016/03/17 at 07:53:08, srirama.m wrote: > > I have removed scheduleTextTrackResurceLoad and directly called honorUser..... > function. All the tests are passing except track-language-preference.html. > > Any quick pointer here why it is failing. > > A guess would be that the scheduleDelayedAction in didAddTrackElement now no > longer trigger honorUser... as before. Right. To follow the spec pendantically, didAddTrackElement would have to queue a task to run the three steps that end with "Honor user preferences for automatic text track selection for this element" and there's also a "await a stable state" FIXME in HTMLTrackElement::scheduleLoad. I think it's fine to change the timing of text track processing as little as possible in this CL, since that will mean that it still always happens after any "await a stable state" bits fixed in HTMLMediaElement, and that should be the same order as before, although not the same timing. Concretely, I think this means renaming m_loadTimer to be only about text track loading, and just dropping m_pendingActionFlags since m_textTrackLoadTimer.isActive() (or something) can serve the same purpose. I've probably overlooked something, but hope that's roughly right :)
philipj@opera.com changed reviewers: + alexclarke@chromium.org, sigbjornf@opera.com
sof@, alexclarke@, you're the top pokers of CancellableTaskFactoryTest.cpp, so I hope you can advise on this situation. In the HTML spec, media elements have their own task source, which is used for queueing tasks for fire events and many other things. At any point, all pending tasks can be canceled, this happens when starting a new resource loads so that tasks/events from a previous resource are not misattributed (by event handlers) as being about the new source. The spec also says "await a stable state" in a few algorithms that can can be aborted with language like "Abort any already-running instance of the resource selection algorithm for this element." This amounts to cancelable microtasks. What do you make of this situation? I see a few options: 1. A media element-specific type of callback object, that can be used for both tasks and microtasks, where HTMLMediaElement keeps track of all pending tasks and has a state bit for each to neuter them, so that their callback becomes a no-op. 2. Adapting CancellableTaskFactory to be able to create any number of pending tasks that can all be canceled as a group. AFAICT the shared type for microtasks and tasks is SameThreadClosure, but CancellableTaskFactory makes WebTaskRunner::Task. Not sure if this is trivial or hard to bridge. 3. Fiddle with task and microtask queues to make it possible to actually remove tasks from them. Presumably this is very hard/messy, but it's worth asking. WDYT? https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: Microtask::enqueueMicrotask(task.release()); On 2016/03/16 17:41:17, Srirama wrote: > Infact when i started i just copied it from ImageLoader but i realized that we > need it for aborting the already running instance. This is the only reason. OK. So this problem has come up several times recently. To make HTMLMediaElement more per spec, we really need both tasks and microtasks that can be canceled. Events should also also be fired using such tasks, replacing the m_asyncEventQueue. Right now the best candidate for all of this is CancellableTaskFactory, but IIRC it can only have one task in flight at a time, because that's just what cancelAndCreate does. I'll add a few reviewers and ask for advice.
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
+skyostil@ On 2016/03/17 11:30:55, philipj_UTC7 wrote: > sof@, alexclarke@, you're the top pokers of CancellableTaskFactoryTest.cpp, so I > hope you can advise on this situation. > > In the HTML spec, media elements have their own task source, which is used for > queueing tasks for fire events and many other things. At any point, all pending > tasks can be canceled, this happens when starting a new resource loads so that > tasks/events from a previous resource are not misattributed (by event handlers) > as being about the new source. > > The spec also says "await a stable state" in a few algorithms that can can be > aborted with language like "Abort any already-running instance of the resource > selection algorithm for this element." This amounts to cancelable microtasks. > > What do you make of this situation? I see a few options: > > 1. A media element-specific type of callback object, that can be used for both > tasks and microtasks, where HTMLMediaElement keeps track of all pending tasks > and has a state bit for each to neuter them, so that their callback becomes a > no-op. > > 2. Adapting CancellableTaskFactory to be able to create any number of pending > tasks that can all be canceled as a group. AFAICT the shared type for microtasks > and tasks is SameThreadClosure, but CancellableTaskFactory makes > WebTaskRunner::Task. Not sure if this is trivial or hard to bridge. The CancellableTaskFactory actually builds a SameThreadClosure, so getting it to instead accept one should be easy. Mass cancellation is possibly a little trickier. Would individual cancellation still be needed? If not then a group could all share the same weak pointer. We'd probably need a new class based on CancellableTaskFactory. > > 3. Fiddle with task and microtask queues to make it possible to actually remove > tasks from them. Presumably this is very hard/messy, but it's worth asking. This is really tricky under the hood since both the chromium base message loop and the render scheduler have been built with the assumption that cancellation is the responsibility of the task itself. Data structures like std::queue and std::priority_queue have been used that don't support arbitrary deletion. > > WDYT? I can think of a few other ways this could be done: 4. We could add a new API that lets blink create a WebTaskRunner for media elements, along with something to allow mass-cancellation of all tasks posted on it. This has some nice side effects for frame blamer and tracing. 5. I'm not sure if it's applicable here, but I have a C++ promises demo that has a dependency graph of tasks (i.e. Task D runs after A, B & C and if you cancel task D then A, B & C no longer need to run). https://codereview.chromium.org/1401553002/ > > https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: > Microtask::enqueueMicrotask(task.release()); > On 2016/03/16 17:41:17, Srirama wrote: > > Infact when i started i just copied it from ImageLoader but i realized that we > > need it for aborting the already running instance. This is the only reason. > > OK. So this problem has come up several times recently. To make HTMLMediaElement > more per spec, we really need both tasks and microtasks that can be canceled. > Events should also also be fired using such tasks, replacing the > m_asyncEventQueue. > > Right now the best candidate for all of this is CancellableTaskFactory, but IIRC > it can only have one task in flight at a time, because that's just what > cancelAndCreate does. I'll add a few reviewers and ask for advice.
> 4. We could add a new API that lets blink create a WebTaskRunner for media > elements, along with something to allow mass-cancellation of all tasks posted on > it. This has some nice side effects for frame blamer and tracing. I like this idea since it would allow us to treat (and schedule) these tasks as a separate unit. However we don't quite have the necessary functionality to do this in the scheduler yet. At minimum I think we'd need a way to create a 'sub task runner' from an existing one that still respects the priority/enable status of its parent. However we probably don't want to block this patch on building all of this. What we could do is implement a self-contained sub task runner in Blink that just delegates tasks to its parent and supports mass cancellation. How does that sound? > 5. I'm not sure if it's applicable here, but I have a C++ promises demo that has > a dependency graph of tasks (i.e. Task D runs after A, B & C and if you cancel > task D then A, B & C no longer need to run). > https://codereview.chromium.org/1401553002/ It's interesting to later consider plugging in promises to whatever we build.
@philip, so i assume this CL can go ahead as per your comment #12. So i have uploaded a new patchset with those changes and code cleanup. Or let me know if we need to wait before we conclude on our discussions. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:879: if (m_textTrackLoadTimer.isActive()) { This is required to keep the order of honorUser.... and loading media resource intact, as in some of the test cases texttrackloadtimer is fired a bit late. Do you think we should still avoid this from here?
On 2016/03/17 12:05:08, alexclarke1 wrote: > +skyostil@ > > On 2016/03/17 11:30:55, philipj_UTC7 wrote: > > sof@, alexclarke@, you're the top pokers of CancellableTaskFactoryTest.cpp, so > I > > hope you can advise on this situation. > > > > In the HTML spec, media elements have their own task source, which is used for > > queueing tasks for fire events and many other things. At any point, all > pending > > tasks can be canceled, this happens when starting a new resource loads so that > > tasks/events from a previous resource are not misattributed (by event > handlers) > > as being about the new source. > > > > The spec also says "await a stable state" in a few algorithms that can can be > > aborted with language like "Abort any already-running instance of the resource > > selection algorithm for this element." This amounts to cancelable microtasks. > > > > What do you make of this situation? I see a few options: > > > > 1. A media element-specific type of callback object, that can be used for both > > tasks and microtasks, where HTMLMediaElement keeps track of all pending tasks > > and has a state bit for each to neuter them, so that their callback becomes a > > no-op. > > > > 2. Adapting CancellableTaskFactory to be able to create any number of pending > > tasks that can all be canceled as a group. AFAICT the shared type for > microtasks > > and tasks is SameThreadClosure, but CancellableTaskFactory makes > > WebTaskRunner::Task. Not sure if this is trivial or hard to bridge. > The CancellableTaskFactory actually builds a SameThreadClosure, so getting it to > instead accept one should be easy. > > Mass cancellation is possibly a little trickier. Would individual cancellation > still be needed? If not then a group could all share the same weak pointer. > We'd probably need a new class based on CancellableTaskFactory. Mass cancelation would suffice for tasks, as those are all in the "media element task source" and are per spec all removed at once. For microtasks, not all instances of "await a stable state" are part of the "resource selection algorithm", which can be aborted. So I think that the microtasks that are part of that algorithm would have to be in a separate buckets, in which case mass cancelation would work, or if they are all in the same bucket, individual cancelation is needed. > > 3. Fiddle with task and microtask queues to make it possible to actually > remove > > tasks from them. Presumably this is very hard/messy, but it's worth asking. > This is really tricky under the hood since both the chromium base message loop > and the render scheduler have been built with the assumption that cancellation > is the responsibility of the task itself. Data structures like std::queue and > std::priority_queue have been used that don't support arbitrary deletion. Yeah, I suspected as much. > > WDYT? > > I can think of a few other ways this could be done: > > 4. We could add a new API that lets blink create a WebTaskRunner for media > elements, along with something to allow mass-cancellation of all tasks posted on > it. This has some nice side effects for frame blamer and tracing. I guess this would mean that all of the tasks in the WebTaskRunner would be processed together, so that if there are two media elements A and B both have two pending tasks, they will always be in AABB order, never ABAB? The spec allows for that, and it's what happens with GenericEventQueue now. This would presumably not have anything to do with the microtasks, but how would such an API look like? > 5. I'm not sure if it's applicable here, but I have a C++ promises demo that has > a dependency graph of tasks (i.e. Task D runs after A, B & C and if you cancel > task D then A, B & C no longer need to run). > https://codereview.chromium.org/1401553002/ I'm also not sure if it would be applicable. For normal tasks mass cancelation will suffice, so it probably wouldn't help.
On 2016/03/17 12:43:55, Sami wrote: > > 4. We could add a new API that lets blink create a WebTaskRunner for media > > elements, along with something to allow mass-cancellation of all tasks posted > on > > it. This has some nice side effects for frame blamer and tracing. > > I like this idea since it would allow us to treat (and schedule) these tasks as > a separate unit. However we don't quite have the necessary functionality to do > this in the scheduler yet. At minimum I think we'd need a way to create a 'sub > task runner' from an existing one that still respects the priority/enable status > of its parent. > > However we probably don't want to block this patch on building all of this. What > we could do is implement a self-contained sub task runner in Blink that just > delegates tasks to its parent and supports mass cancellation. How does that > sound? Just to spell it out, that would be a thing to use where the spec says "queue a task" and the task source is the "media element task source", correct? If so that would be great for getting rid of GenericEventQueue and getting all normal tasks into the same queue, but wouldn't have any relevance for microtasks. Maybe it's best to just treat them as entirely separate questions. Do you have a rough sketch of how a self-contained sub task runner would look? Would it be a lot like GenericEventQueue in that it's created and owned by a HTMLMediaElement?
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:879: if (m_textTrackLoadTimer.isActive()) { On 2016/03/18 07:38:11, Srirama wrote: > This is required to keep the order of honorUser.... and loading media resource > intact, as in some of the test cases texttrackloadtimer is fired a bit late. Do > you think we should still avoid this from here? Hmm, which test case is that?
On 2016/03/18 07:38:11, Srirama wrote: > @philip, so i assume this CL can go ahead as per your comment #12. So i have > uploaded a new patchset with those changes and code cleanup. Or let me know if > we need to wait before we conclude on our discussions. Yeah, it looks like we shouldn't block this CL. We do need a cancelable microtask here, will see if it can be simplified at all or if this is just what it has to be.
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:272: class HTMLMediaElement::Task : public WebTaskRunner::Task { It looks like it should be possible to use CancellableTaskFactory for this, cancelAndCreate() returns a WebTaskRunner::Task and that can be passed to enqueueMicrotask. Did you try that and it all falls apart because the ScriptState tracking mentioned by TODO(jochen) in Microtask.h?
On 2016/03/23 06:48:57, philipj_UTC7 wrote: > > 4. We could add a new API that lets blink create a WebTaskRunner for media > > elements, along with something to allow mass-cancellation of all tasks posted > on > > it. This has some nice side effects for frame blamer and tracing. > > I guess this would mean that all of the tasks in the WebTaskRunner would be > processed together, so that if there are two media elements A and B both have > two pending tasks, they will always be in AABB order, never ABAB? The spec > allows for that, and it's what happens with GenericEventQueue now. The only rule is that (immediate) tasks in a single queue will always run in their posted order. So in this case it could be AABB or ABAB depending on when the tasks were posted. Since it sounds like the spec gives us a lot of leeway here, it might be best to create one task queue per element so that the ordering constraint is as loose as possible. > This would presumably not have anything to do with the microtasks, but how would > such an API look like? Right, microtasks are separate. I could imagine something like: // Create a task runner associated with our frame. OwnPtr<WebTaskRunner> mediaTaskRunner = webFrameScheduler->createTaskRunner(); // Schedule a task. mediaTaskRunner->postTask(BLINK_FROM_HERE, ...); // Cancel all outstanding tasks. mediaTaskRunner->cancelAllTasks(); (N.B. I'm not sure if we can make all WebTaskRunners cancellable or do we need a new interface for that.)
(Filed https://bugs.chromium.org/p/chromium/issues/detail?id=597240 for the new task runner interface.)
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:298: ScriptState::Scope scope(m_scriptState.get()); What break if there is no ScriptState::Scope here? https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:884: if (m_loadState == LoadingFromSourceElement) This doesn't look right. The spec decides whether to use the src attributes or source child elements after awaiting a stable state. Overall I would expect the steps here to simply match steps 5 and onward. If it actually does, then documenting why would be good, so that it can be later refactored to more obviously match the spec. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: enqueueMicrotask(); I guess this is the "Await a stable state" after "Failed with elements"? If continueResourceSelectionAlgorithm() is made to match step 5 and onward, this will have to change as well. A specific function for each continuation point of the algorithm is probably the most clear. I'm guessing that m_loadState won't be needed at all, but if it really is, then perhaps setting that state explicitly here or ASSERTing that it's in the appropriate state would work too. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2831: enqueueMicrotask(); This should actually synchronously jump to the "Find next candidate" step. I think there should be tests in web-platform-tests that fail as long as that isn't the case, I remember simonp@opera.com writing a lot of those. The trouble here is to use some other mechanism than recursion, or too many source elements will result in a stack overflow. Either an actual goto, a do { ... continue; ... } while(false), a return value that means "call this function again" could work.
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:272: class HTMLMediaElement::Task : public WebTaskRunner::Task { On 2016/03/23 07:27:18, philipj_UTC7 wrote: > It looks like it should be possible to use CancellableTaskFactory for this, > cancelAndCreate() returns a WebTaskRunner::Task and that can be passed to > enqueueMicrotask. Did you try that and it all falls apart because the > ScriptState tracking mentioned by TODO(jochen) in Microtask.h? I haven't tried that, do you want me to give a try inspite of Jochen's TODO comments? https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:298: ScriptState::Scope scope(m_scriptState.get()); On 2016/03/24 04:43:05, philipj_UTC7 wrote: > What break if there is no ScriptState::Scope here? Tested locally by removing and no cases are failing. I will remove it. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:879: if (m_textTrackLoadTimer.isActive()) { On 2016/03/23 07:12:02, philipj_UTC7 wrote: > On 2016/03/18 07:38:11, Srirama wrote: > > This is required to keep the order of honorUser.... and loading media resource > > intact, as in some of the test cases texttrackloadtimer is fired a bit late. > Do > > you think we should still avoid this from here? > > Hmm, which test case is that? Sorry, couldn't remember exactly which one is failing. Removed it and now all cases are passing. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:884: if (m_loadState == LoadingFromSourceElement) On 2016/03/24 04:43:05, philipj_UTC7 wrote: > This doesn't look right. The spec decides whether to use the src attributes or > source child elements after awaiting a stable state. Overall I would expect the > steps here to simply match steps 5 and onward. If it actually does, then > documenting why would be good, so that it can be later refactored to more > obviously match the spec. Hmm, probably this can simply be loadInternal() based on the decision for the below comments. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: enqueueMicrotask(); On 2016/03/24 04:43:05, philipj_UTC7 wrote: > I guess this is the "Await a stable state" after "Failed with elements"? If > continueResourceSelectionAlgorithm() is made to match step 5 and onward, this > will have to change as well. A specific function for each continuation point of > the algorithm is probably the most clear. I'm guessing that m_loadState won't > be needed at all, but if it really is, then perhaps setting that state > explicitly here or ASSERTing that it's in the appropriate state would work too. Probably some more refactoring is needed before i can remove m_loadState. For ex, we need to differentiate whether mediaLoadingFailed is called for 'source' or 'src'. And m_loadState is already validated above, so do we need to set/assert here again? And if we make different functions for different continuation points, do you want me to call those functions based on m_loadState in HTMLMediaElement::Task::run function? https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2831: enqueueMicrotask(); On 2016/03/24 04:43:05, philipj_UTC7 wrote: > This should actually synchronously jump to the "Find next candidate" step. I > think there should be tests in web-platform-tests that fail as long as that > isn't the case, I remember mailto:simonp@opera.com writing a lot of those. > > The trouble here is to use some other mechanism than recursion, or too many > source elements will result in a stack overflow. Either an actual goto, a do { > ... continue; ... } while(false), a return value that means "call this function > again" could work. This looks a bit confusing, this function sourceWasAdded() will be called from script, so how can we synchronously go back to the "Find next candidate" step? We are already invoking resource selection algo at the start of this function as per spec https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element. Probably this function also needs some refactoring?
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:272: class HTMLMediaElement::Task : public WebTaskRunner::Task { On 2016/03/29 11:26:29, Srirama wrote: > On 2016/03/23 07:27:18, philipj_UTC7 wrote: > > It looks like it should be possible to use CancellableTaskFactory for this, > > cancelAndCreate() returns a WebTaskRunner::Task and that can be passed to > > enqueueMicrotask. Did you try that and it all falls apart because the > > ScriptState tracking mentioned by TODO(jochen) in Microtask.h? > > I haven't tried that, do you want me to give a try inspite of Jochen's TODO > comments? Yeah, sure, I wouldn't assume that the ScriptState is actually needed, and in fact if it is it could be a bug. (Because the most obvious thing one might need a ScriptState for is to enqueue another microtask, and I don't think there's any place per spec where that should happen in the sync steps after "await a stable state".) https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: enqueueMicrotask(); On 2016/03/29 11:26:29, Srirama wrote: > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > I guess this is the "Await a stable state" after "Failed with elements"? If > > continueResourceSelectionAlgorithm() is made to match step 5 and onward, this > > will have to change as well. A specific function for each continuation point > of > > the algorithm is probably the most clear. I'm guessing that m_loadState won't > > be needed at all, but if it really is, then perhaps setting that state > > explicitly here or ASSERTing that it's in the appropriate state would work > too. > > Probably some more refactoring is needed before i can remove m_loadState. For > ex, we need to differentiate whether mediaLoadingFailed is called for 'source' > or 'src'. > And m_loadState is already validated above, so do we need to set/assert here > again? > And if we make different functions for different continuation points, do you > want me to call those functions based on m_loadState in > HTMLMediaElement::Task::run function? Which bits do you mean for "m_loadState is already validated above"? I think the ideal state of things would be separate functions for every point where the algorithm can continue, and that WTF::bind or similar is used to schedule those continuations. I think keeping m_loadState around to differentiate between src and src on error seems OK. One might consider renaming it to correspond to the mode in https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... but that would change with srcObject so perhaps it's best to just leave that as-is for now. If the WaitingForSource entry ends up unused it could be removed. I would probably suggest first checking if CancellableTaskFactory and WTF::bind can be used together to replace the current inner class Task. If it can, then it probably isn't too hard to continue with the changes I suggested. But if not, then plan B would be more state. It could be an enum that basically keeps track of where in the resource selection algorithm we currently are. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2831: enqueueMicrotask(); On 2016/03/29 11:26:29, Srirama wrote: > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > This should actually synchronously jump to the "Find next candidate" step. I > > think there should be tests in web-platform-tests that fail as long as that > > isn't the case, I remember mailto:simonp@opera.com writing a lot of those. > > > > The trouble here is to use some other mechanism than recursion, or too many > > source elements will result in a stack overflow. Either an actual goto, a do { > > ... continue; ... } while(false), a return value that means "call this > function > > again" could work. > > This looks a bit confusing, this function sourceWasAdded() will be called from > script, so how can we synchronously go back to the "Find next candidate" step? > We are already invoking resource selection algo at the start of this function as > per spec > https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element. > Probably this function also needs some refactoring? Yes, the spec expresses this as a single big algorithm where we can end up waiting. What we'd need is to make sure that the steps beginning at "find next candidate" are in a separate function that be called here. With extra care taken so that it's never called recursively in the case or many source elements.
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: enqueueMicrotask(); On 2016/03/29 12:39:32, philipj_UTC7 wrote: > On 2016/03/29 11:26:29, Srirama wrote: > > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > > I guess this is the "Await a stable state" after "Failed with elements"? If > > > continueResourceSelectionAlgorithm() is made to match step 5 and onward, > this > > > will have to change as well. A specific function for each continuation point > > of > > > the algorithm is probably the most clear. I'm guessing that m_loadState > won't > > > be needed at all, but if it really is, then perhaps setting that state > > > explicitly here or ASSERTing that it's in the appropriate state would work > > too. > > > > Probably some more refactoring is needed before i can remove m_loadState. For > > ex, we need to differentiate whether mediaLoadingFailed is called for 'source' > > or 'src'. > > And m_loadState is already validated above, so do we need to set/assert here > > again? > > And if we make different functions for different continuation points, do you > > want me to call those functions based on m_loadState in > > HTMLMediaElement::Task::run function? > > Which bits do you mean for "m_loadState is already validated above"? > > I think the ideal state of things would be separate functions for every point > where the algorithm can continue, and that WTF::bind or similar is used to > schedule those continuations. I think keeping m_loadState around to > differentiate between src and src on error seems OK. One might consider renaming > it to correspond to the mode in > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > but that would change with srcObject so perhaps it's best to just leave that > as-is for now. If the WaitingForSource entry ends up unused it could be removed. > > I would probably suggest first checking if CancellableTaskFactory and WTF::bind > can be used together to replace the current inner class Task. If it can, then it > probably isn't too hard to continue with the changes I suggested. But if not, > then plan B would be more state. It could be an enum that basically keeps track > of where in the resource selection algorithm we currently are. This is actually part of mediaLoadingFailed() and i am referring to the conditon at the start of the function if (m_readyState < HAVE_METADATA && m_loadState == LoadingFromSourceElement). These lines are shrinked in the review window.
https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: enqueueMicrotask(); On 2016/03/29 12:49:12, Srirama wrote: > On 2016/03/29 12:39:32, philipj_UTC7 wrote: > > On 2016/03/29 11:26:29, Srirama wrote: > > > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > > > I guess this is the "Await a stable state" after "Failed with elements"? > If > > > > continueResourceSelectionAlgorithm() is made to match step 5 and onward, > > this > > > > will have to change as well. A specific function for each continuation > point > > > of > > > > the algorithm is probably the most clear. I'm guessing that m_loadState > > won't > > > > be needed at all, but if it really is, then perhaps setting that state > > > > explicitly here or ASSERTing that it's in the appropriate state would work > > > too. > > > > > > Probably some more refactoring is needed before i can remove m_loadState. > For > > > ex, we need to differentiate whether mediaLoadingFailed is called for > 'source' > > > or 'src'. > > > And m_loadState is already validated above, so do we need to set/assert here > > > again? > > > And if we make different functions for different continuation points, do you > > > want me to call those functions based on m_loadState in > > > HTMLMediaElement::Task::run function? > > > > Which bits do you mean for "m_loadState is already validated above"? > > > > I think the ideal state of things would be separate functions for every point > > where the algorithm can continue, and that WTF::bind or similar is used to > > schedule those continuations. I think keeping m_loadState around to > > differentiate between src and src on error seems OK. One might consider > renaming > > it to correspond to the mode in > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > but that would change with srcObject so perhaps it's best to just leave that > > as-is for now. If the WaitingForSource entry ends up unused it could be > removed. > > > > I would probably suggest first checking if CancellableTaskFactory and > WTF::bind > > can be used together to replace the current inner class Task. If it can, then > it > > probably isn't too hard to continue with the changes I suggested. But if not, > > then plan B would be more state. It could be an enum that basically keeps > track > > of where in the resource selection algorithm we currently are. > > This is actually part of mediaLoadingFailed() and i am referring to the conditon > at the start of the function > if (m_readyState < HAVE_METADATA && m_loadState == LoadingFromSourceElement). > These lines are shrinked in the review window. Oh OK. Yeah in that case it would obviously make no sense to assert anything about m_loadState or setting it to the same value again :)
Patchset #3 (id:40001) has been deleted
PTAL. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:272: class HTMLMediaElement::Task : public WebTaskRunner::Task { On 2016/03/29 12:39:32, philipj_sick_March30 wrote: > On 2016/03/29 11:26:29, Srirama wrote: > > On 2016/03/23 07:27:18, philipj_UTC7 wrote: > > > It looks like it should be possible to use CancellableTaskFactory for this, > > > cancelAndCreate() returns a WebTaskRunner::Task and that can be passed to > > > enqueueMicrotask. Did you try that and it all falls apart because the > > > ScriptState tracking mentioned by TODO(jochen) in Microtask.h? > > > > I haven't tried that, do you want me to give a try inspite of Jochen's TODO > > comments? > > Yeah, sure, I wouldn't assume that the ScriptState is actually needed, and in > fact if it is it could be a bug. (Because the most obvious thing one might need > a ScriptState for is to enqueue another microtask, and I don't think there's any > place per spec where that should happen in the sync steps after "await a stable > state".) Done. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: enqueueMicrotask(); On 2016/03/29 12:57:51, philipj_sick_March30 wrote: > On 2016/03/29 12:49:12, Srirama wrote: > > On 2016/03/29 12:39:32, philipj_UTC7 wrote: > > > On 2016/03/29 11:26:29, Srirama wrote: > > > > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > > > > I guess this is the "Await a stable state" after "Failed with elements"? > > If > > > > > continueResourceSelectionAlgorithm() is made to match step 5 and onward, > > > this > > > > > will have to change as well. A specific function for each continuation > > point > > > > of > > > > > the algorithm is probably the most clear. I'm guessing that m_loadState > > > won't > > > > > be needed at all, but if it really is, then perhaps setting that state > > > > > explicitly here or ASSERTing that it's in the appropriate state would > work > > > > too. > > > > > > > > Probably some more refactoring is needed before i can remove m_loadState. > > For > > > > ex, we need to differentiate whether mediaLoadingFailed is called for > > 'source' > > > > or 'src'. > > > > And m_loadState is already validated above, so do we need to set/assert > here > > > > again? > > > > And if we make different functions for different continuation points, do > you > > > > want me to call those functions based on m_loadState in > > > > HTMLMediaElement::Task::run function? > > > > > > Which bits do you mean for "m_loadState is already validated above"? > > > > > > I think the ideal state of things would be separate functions for every > point > > > where the algorithm can continue, and that WTF::bind or similar is used to > > > schedule those continuations. I think keeping m_loadState around to > > > differentiate between src and src on error seems OK. One might consider > > renaming > > > it to correspond to the mode in > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > but that would change with srcObject so perhaps it's best to just leave that > > > as-is for now. If the WaitingForSource entry ends up unused it could be > > removed. > > > > > > I would probably suggest first checking if CancellableTaskFactory and > > WTF::bind > > > can be used together to replace the current inner class Task. If it can, > then > > it > > > probably isn't too hard to continue with the changes I suggested. But if > not, > > > then plan B would be more state. It could be an enum that basically keeps > > track > > > of where in the resource selection algorithm we currently are. > > > > This is actually part of mediaLoadingFailed() and i am referring to the > conditon > > at the start of the function > > if (m_readyState < HAVE_METADATA && m_loadState == LoadingFromSourceElement). > > These lines are shrinked in the review window. > > Oh OK. Yeah in that case it would obviously make no sense to assert anything > about m_loadState or setting it to the same value again :) Done. Tried removing WaitingForSource blindly and some <source> element related tests are failing (media-source-append-multiple.html). Probably needs more refactoring to map it to 'mode' as you mentioned. https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2831: enqueueMicrotask(); On 2016/03/29 12:39:32, philipj_sick_March30 wrote: > On 2016/03/29 11:26:29, Srirama wrote: > > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > > This should actually synchronously jump to the "Find next candidate" step. I > > > think there should be tests in web-platform-tests that fail as long as that > > > isn't the case, I remember mailto:simonp@opera.com writing a lot of those. > > > > > > The trouble here is to use some other mechanism than recursion, or too many > > > source elements will result in a stack overflow. Either an actual goto, a do > { > > > ... continue; ... } while(false), a return value that means "call this > > function > > > again" could work. > > > > This looks a bit confusing, this function sourceWasAdded() will be called from > > script, so how can we synchronously go back to the "Find next candidate" step? > > We are already invoking resource selection algo at the start of this function > as > > per spec > > > https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element. > > Probably this function also needs some refactoring? > > Yes, the spec expresses this as a single big algorithm where we can end up > waiting. What we'd need is to make sure that the steps beginning at "find next > candidate" are in a separate function that be called here. With extra care taken > so that it's never called recursively in the case or many source elements. I thought loadNextSourceChild() is more appropriate for "Find next candidate" section, so i just called. And regarding recursion, sorry i couldn't exactly understand how it can go into a recursion. Here in sourceWasAdded we are calling loadNextSourceChild() and i couldn't see from where loadNextSourceChild() will be called again and again from inside. There is a "await a stable state" (mediaLoadingFailed) before loadNextSourceChild() gets called again. Please let me know if i miss something in understanding your point.
On 2016/03/30 11:35:40, Srirama wrote: > PTAL. > > https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:272: class > HTMLMediaElement::Task : public WebTaskRunner::Task { > On 2016/03/29 12:39:32, philipj_sick_March30 wrote: > > On 2016/03/29 11:26:29, Srirama wrote: > > > On 2016/03/23 07:27:18, philipj_UTC7 wrote: > > > > It looks like it should be possible to use CancellableTaskFactory for > this, > > > > cancelAndCreate() returns a WebTaskRunner::Task and that can be passed to > > > > enqueueMicrotask. Did you try that and it all falls apart because the > > > > ScriptState tracking mentioned by TODO(jochen) in Microtask.h? > > > > > > I haven't tried that, do you want me to give a try inspite of Jochen's TODO > > > comments? > > > > Yeah, sure, I wouldn't assume that the ScriptState is actually needed, and in > > fact if it is it could be a bug. (Because the most obvious thing one might > need > > a ScriptState for is to enqueue another microtask, and I don't think there's > any > > place per spec where that should happen in the sync steps after "await a > stable > > state".) > > Done. > > https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1431: > enqueueMicrotask(); > On 2016/03/29 12:57:51, philipj_sick_March30 wrote: > > On 2016/03/29 12:49:12, Srirama wrote: > > > On 2016/03/29 12:39:32, philipj_UTC7 wrote: > > > > On 2016/03/29 11:26:29, Srirama wrote: > > > > > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > > > > > I guess this is the "Await a stable state" after "Failed with > elements"? > > > If > > > > > > continueResourceSelectionAlgorithm() is made to match step 5 and > onward, > > > > this > > > > > > will have to change as well. A specific function for each continuation > > > point > > > > > of > > > > > > the algorithm is probably the most clear. I'm guessing that > m_loadState > > > > won't > > > > > > be needed at all, but if it really is, then perhaps setting that state > > > > > > explicitly here or ASSERTing that it's in the appropriate state would > > work > > > > > too. > > > > > > > > > > Probably some more refactoring is needed before i can remove > m_loadState. > > > For > > > > > ex, we need to differentiate whether mediaLoadingFailed is called for > > > 'source' > > > > > or 'src'. > > > > > And m_loadState is already validated above, so do we need to set/assert > > here > > > > > again? > > > > > And if we make different functions for different continuation points, do > > you > > > > > want me to call those functions based on m_loadState in > > > > > HTMLMediaElement::Task::run function? > > > > > > > > Which bits do you mean for "m_loadState is already validated above"? > > > > > > > > I think the ideal state of things would be separate functions for every > > point > > > > where the algorithm can continue, and that WTF::bind or similar is used to > > > > schedule those continuations. I think keeping m_loadState around to > > > > differentiate between src and src on error seems OK. One might consider > > > renaming > > > > it to correspond to the mode in > > > > > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > > but that would change with srcObject so perhaps it's best to just leave > that > > > > as-is for now. If the WaitingForSource entry ends up unused it could be > > > removed. > > > > > > > > I would probably suggest first checking if CancellableTaskFactory and > > > WTF::bind > > > > can be used together to replace the current inner class Task. If it can, > > then > > > it > > > > probably isn't too hard to continue with the changes I suggested. But if > > not, > > > > then plan B would be more state. It could be an enum that basically keeps > > > track > > > > of where in the resource selection algorithm we currently are. > > > > > > This is actually part of mediaLoadingFailed() and i am referring to the > > conditon > > > at the start of the function > > > if (m_readyState < HAVE_METADATA && m_loadState == > LoadingFromSourceElement). > > > These lines are shrinked in the review window. > > > > Oh OK. Yeah in that case it would obviously make no sense to assert anything > > about m_loadState or setting it to the same value again :) > > Done. Tried removing WaitingForSource blindly and some <source> element related > tests are failing (media-source-append-multiple.html). Probably needs more > refactoring to map it to 'mode' as you mentioned. > > https://codereview.chromium.org/1810513002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2831: > enqueueMicrotask(); > On 2016/03/29 12:39:32, philipj_sick_March30 wrote: > > On 2016/03/29 11:26:29, Srirama wrote: > > > On 2016/03/24 04:43:05, philipj_UTC7 wrote: > > > > This should actually synchronously jump to the "Find next candidate" step. > I > > > > think there should be tests in web-platform-tests that fail as long as > that > > > > isn't the case, I remember mailto:simonp@opera.com writing a lot of those. > > > > > > > > The trouble here is to use some other mechanism than recursion, or too > many > > > > source elements will result in a stack overflow. Either an actual goto, a > do > > { > > > > ... continue; ... } while(false), a return value that means "call this > > > function > > > > again" could work. > > > > > > This looks a bit confusing, this function sourceWasAdded() will be called > from > > > script, so how can we synchronously go back to the "Find next candidate" > step? > > > We are already invoking resource selection algo at the start of this > function > > as > > > per spec > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element. > > > Probably this function also needs some refactoring? > > > > Yes, the spec expresses this as a single big algorithm where we can end up > > waiting. What we'd need is to make sure that the steps beginning at "find next > > candidate" are in a separate function that be called here. With extra care > taken > > so that it's never called recursively in the case or many source elements. > > I thought loadNextSourceChild() is more appropriate for "Find next candidate" > section, so i just called. And regarding recursion, sorry i couldn't exactly > understand how it can go into a recursion. Here in sourceWasAdded we are calling > loadNextSourceChild() and i couldn't see from where loadNextSourceChild() will > be called again and again from inside. There is a "await a stable state" > (mediaLoadingFailed) before loadNextSourceChild() gets called again. Please let > me know if i miss something in understanding your point. Ping.
I love the way this is going, more and more like the spec :) I think you're missing some place where you actually cancel the pending microtasks corresponding to "Abort any already-running instance of the resource selection algorithm for this element" in the spec? https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1368: Microtask::enqueueMicrotask(WTF::bind(&HTMLMediaElement::loadNextSourceChild, this)); Per spec the "await a stable state" is before forgetResourceSpecificTracks(), could you try to move it up to get the same effect? Note that you'll have to make this microtask cancelable as well, and cancel it if the resource selection algorithm is aborted. https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2768: loadNextSourceChild(); If there are many source children, will this recurse on the stack? Can you make it crash by having 10000 of them? https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3182: if (m_resourceSelectionAlgorithmContinuationTask->isPending()) Now that we're in an Oilpan only world I'm not entirely sure this is needed. Will the pending task itself not have a strong reference to the HTMLMediaElement object, or is it weak?
Patchset #4 (id:80001) has been deleted
I have intentionally not added abort step, as it is already being cancelled as part of cancelAndCreate. But i think it is good to have it as per spec, so i have added it again. https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1368: Microtask::enqueueMicrotask(WTF::bind(&HTMLMediaElement::loadNextSourceChild, this)); On 2016/04/06 12:06:52, philipj wrote: > Per spec the "await a stable state" is before forgetResourceSpecificTracks(), > could you try to move it up to get the same effect? Note that you'll have to > make this microtask cancelable as well, and cancel it if the resource selection > algorithm is aborted. Done. https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2768: loadNextSourceChild(); On 2016/04/06 12:06:52, philipj wrote: > If there are many source children, will this recurse on the stack? Can you make > it crash by having 10000 of them? I wrote a test content like below. ********************* audio = document.getElementById('a'); var audioFile = findMediaFile("audio", "content/test"); for (var i = 0; i < 10; i++) { source = document.createElement("source"); source.setAttribute("src", "invalid.mp3"); audio.appendChild(source); } source = document.createElement("source"); source.setAttribute("src", audioFile); audio.appendChild(source); *********************** loadNextSourceChild is never called from sourceWasAdded function (it bails out early from one of the conditions), probably i will change the test case, but i still feel that if it reaches to loadNextSourceChild step here it would have finished previous source child processing. https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3182: if (m_resourceSelectionAlgorithmContinuationTask->isPending()) On 2016/04/06 12:06:52, philipj wrote: > Now that we're in an Oilpan only world I'm not entirely sure this is needed. > Will the pending task itself not have a strong reference to the HTMLMediaElement > object, or is it weak? I think it will be a strong pointer (ownptr), isn't it. I will remove it.
On 2016/04/11 08:08:00, Srirama wrote: > I have intentionally not added abort step, as it is already being cancelled as > part of cancelAndCreate. But i think it is good to have it as per spec, so i > have added it again. > > https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1368: > Microtask::enqueueMicrotask(WTF::bind(&HTMLMediaElement::loadNextSourceChild, > this)); > On 2016/04/06 12:06:52, philipj wrote: > > Per spec the "await a stable state" is before forgetResourceSpecificTracks(), > > could you try to move it up to get the same effect? Note that you'll have to > > make this microtask cancelable as well, and cancel it if the resource > selection > > algorithm is aborted. > > Done. > > https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2768: > loadNextSourceChild(); > On 2016/04/06 12:06:52, philipj wrote: > > If there are many source children, will this recurse on the stack? Can you > make > > it crash by having 10000 of them? > > I wrote a test content like below. > ********************* > audio = document.getElementById('a'); > var audioFile = findMediaFile("audio", "content/test"); > for (var i = 0; i < 10; i++) { > source = document.createElement("source"); > source.setAttribute("src", "invalid.mp3"); > audio.appendChild(source); > } > > source = document.createElement("source"); > source.setAttribute("src", audioFile); > audio.appendChild(source); > *********************** > loadNextSourceChild is never called from sourceWasAdded function (it bails out > early from one of the conditions), probably i will change the test case, but i > still feel that if it reaches to loadNextSourceChild step here it would have > finished previous source child processing. > > https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3182: if > (m_resourceSelectionAlgorithmContinuationTask->isPending()) > On 2016/04/06 12:06:52, philipj wrote: > > Now that we're in an Oilpan only world I'm not entirely sure this is needed. > > Will the pending task itself not have a strong reference to the > HTMLMediaElement > > object, or is it weak? > > I think it will be a strong pointer (ownptr), isn't it. I will remove it. I have modified the test case to make sure it enters loadNextSourceChild() from sourceWasAdded(), but it doesn't recurse even if i add 100000 source elements. Please find below my test case. ********************************************* <!DOCTYPE html> <video id="v"></video> <script src=media-file.js></script> <script> var video, source; var count = 1; video = document.getElementById('v'); source = document.createElement("source"); source.setAttribute("src", count + "-invalid.mp3"); video.appendChild(source); var Error = function () { count++; video.removeChild(source); source = document.createElement("source"); source.setAttribute("src", count + "-invalid.mp3"); video.appendChild(source); if (count < 3) source.onerror = Error; }; source.onerror = Error; </script> ********************************************************************
My nick says I'm slow, and it's true! I think this looks good now, can you rebase and start the bots to check? And can you write a test for many source elements that would per spec walk past 10000 source elements inside a single microtask just to show that it works? https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2768: loadNextSourceChild(); On 2016/04/11 08:07:59, Srirama wrote: > On 2016/04/06 12:06:52, philipj wrote: > > If there are many source children, will this recurse on the stack? Can you > make > > it crash by having 10000 of them? > > I wrote a test content like below. > ********************* > audio = document.getElementById('a'); > var audioFile = findMediaFile("audio", "content/test"); > for (var i = 0; i < 10; i++) { > source = document.createElement("source"); > source.setAttribute("src", "invalid.mp3"); > audio.appendChild(source); > } > > source = document.createElement("source"); > source.setAttribute("src", audioFile); > audio.appendChild(source); > *********************** > loadNextSourceChild is never called from sourceWasAdded function (it bails out > early from one of the conditions), probably i will change the test case, but i > still feel that if it reaches to loadNextSourceChild step here it would have > finished previous source child processing. Can you change the test so that it appends 10000 plain source elements with no attributes or anything, and then a source element that would actually load? When you get to the source element with the src attribute, what does the stack look like? https://codereview.chromium.org/1810513002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:876: void HTMLMediaElement::enqueueMicrotaskForResourceSelectionAlgorithmContinuation() This is called in just a single place, inline it? https://codereview.chromium.org/1810513002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:982: void HTMLMediaElement::loadNextSourceChildOnError() Maybe AfterError since it happens after awaiting a stable state?
Patchset #5 (id:120001) has been deleted
On 2016/04/19 14:52:45, philipj_slow wrote: > My nick says I'm slow, and it's true! I think this looks good now, can you > rebase and start the bots to check? > > And can you write a test for many source elements that would per spec walk past > 10000 source elements inside a single microtask just to show that it works? > > https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1810513002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2768: > loadNextSourceChild(); > On 2016/04/11 08:07:59, Srirama wrote: > > On 2016/04/06 12:06:52, philipj wrote: > > > If there are many source children, will this recurse on the stack? Can you > > make > > > it crash by having 10000 of them? > > > > I wrote a test content like below. > > ********************* > > audio = document.getElementById('a'); > > var audioFile = findMediaFile("audio", "content/test"); > > for (var i = 0; i < 10; i++) { > > source = document.createElement("source"); > > source.setAttribute("src", "invalid.mp3"); > > audio.appendChild(source); > > } > > > > source = document.createElement("source"); > > source.setAttribute("src", audioFile); > > audio.appendChild(source); > > *********************** > > loadNextSourceChild is never called from sourceWasAdded function (it bails out > > early from one of the conditions), probably i will change the test case, but i > > still feel that if it reaches to loadNextSourceChild step here it would have > > finished previous source child processing. > > Can you change the test so that it appends 10000 plain source elements with no > attributes or anything, and then a source element that would actually load? When > you get to the source element with the src attribute, what does the stack look > like? > > https://codereview.chromium.org/1810513002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1810513002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:876: void > HTMLMediaElement::enqueueMicrotaskForResourceSelectionAlgorithmContinuation() > This is called in just a single place, inline it? > > https://codereview.chromium.org/1810513002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:982: void > HTMLMediaElement::loadNextSourceChildOnError() > Maybe AfterError since it happens after awaiting a stable state? I have modified the test like below. ****************************************************************** <!DOCTYPE html> <video></video> <script src=media-file.js></script> <script> var source; var video = document.querySelector("video"); for (var i = 0; i < 10000; i++) { source = document.createElement("source"); video.appendChild(source); } source = document.createElement("source"); source.src = findMediaFile("video", "content/test"); video.appendChild(source); </script> ***************************************************************** Callstack when the "source" element with "src" attribute hits sourceWasAdded() function ****************************************************************** #0 blink::HTMLMediaElement::sourceWasAdded(blink::HTMLSourceElement*) () at ../../third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2728 #1 0x00007fde54609f68 in blink::HTMLSourceElement::insertedInto(blink::ContainerNode*) () at ../../third_party/WebKit/Source/core/html/HTMLSourceElement.cpp:104 #2 0x00007fde541a5535 in blink::ContainerNode::notifyNodeInsertedInternal(blink::Node&, blink::HeapVector<blink::Member<blink::Node>, 11ul>&) () at ../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:712 #3 0x00007fde541a3bb1 in blink::ContainerNode::notifyNodeInserted(blink::Node&, blink::ContainerNode::ChildrenChangeSource) () at ../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:692 #4 0x00007fde541a3307 in blink::ContainerNode::updateTreeAfterInsertion(blink::Node&) () at ../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:1163 #5 0x00007fde541a2a7c in blink::ContainerNode::appendChild(blink::Node*, blink::ExceptionState&) () at ../../third_party/WebKit/Source/core/dom/ContainerNode.cpp:644 #6 0x00007fde542b2f3a in blink::Node::appendChild(blink::Node*, blink::ExceptionState&) () at ../../third_party/WebKit/Source/core/dom/Node.cpp:413 #7 0x00007fde535530e1 in blink::NodeV8Internal::appendChildMethodForMainWorld(v8::FunctionCallbackInfo<v8::Value> const&) () at gen/blink/bindings/core/v8/V8Node.cpp:691 #8 0x00007fde5354f545 in blink::NodeV8Internal::appendChildMethodCallbackForMainWorld(v8::FunctionCallbackInfo<v8::Value> const&) () at gen/blink/bindings/core/v8/V8Node.cpp:701 #9 0x00007fde5e14e3bb in v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) () at ../../v8/src/api-arguments.cc:16 #10 0x00007fde5e19e204 in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>) () at ../../v8/src/builtins.cc:4556 #11 0x00007fde5e1d2827 in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) () at ../../v8/src/builtins.cc:4574 #12 0x00007fde5e1aaf4f in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) () at ../../v8/src/builtins.cc:4571 #13 0x0000355eaf408967 in ?? () #14 0x0000355eaf4088a1 in ?? () #15 0x0000355eaf4088a1 in ?? () #16 0x00007fffb98778f0 in ?? () #17 0x0000000300000000 in ?? () #18 0x00007fffb9877958 in ?? () #19 0x0000355eaf550aa6 in ?? () #20 0x0000185788afc3b9 in ?? () #21 0x00003d0b11385779 in ?? () #22 0x00003d0b113225f1 in ?? () #23 0x0000185788afc3b9 in ?? () #24 0x00003d0b11385751 in ?? () #25 0x00001c184c10b6c9 in ?? () #26 0x0000185788a8b6f9 in ?? () #27 0x00007fffb9877988 in ?? () #28 0x0000355eaf444aa3 in ?? () #29 0x0000147031cb8059 in ?? () #30 0x00001c184c10b6c9 in ?? () #31 0x0000355eaf4449c1 in ?? () #32 0x0000000c00000000 in ?? () #33 0x00007fffb98779f0 in ?? () #34 0x0000355eaf415cef in ?? () #35 0x0000000000000000 in ?? () *************************************************************************** And regarding layout test failures, media tests are failing, others are imported tests which may need rebaselining. media/video-controls-in-media-document.html media/media-document-audio-repaint.html Both these tests are failing because video is failed to load. https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... BufferedResourceLoader start is failing. I will debug further and find out why is it failing.
Thanks Srirama, from that and looking at the resource selection algorithm in the spec I'm pretty sure that there is no stack overflow hiding here. The implementation isn't quite similar to the spec, but the problem would have been if selectNextSourceChild was recursive or similar setup. So let's not worry more about that until such a problem appears in reality, which is hopefully never. If you address the other issues I think should be good for landing.
fs@opera.com changed reviewers: + fs@opera.com
On 2016/04/27 13:28:09, philipj_slow wrote: > Thanks Srirama, from that and looking at the resource selection algorithm in the > spec I'm pretty sure that there is no stack overflow hiding here. The > implementation isn't quite similar to the spec, but the problem would have been > if selectNextSourceChild was recursive or similar setup. > > So let's not worry more about that until such a problem appears in reality, > which is hopefully never. > > If you address the other issues I think should be good for landing. Thanks philip. I was checking the two layout failure issues, and looks like the request is getting cancalled. This is getting cancelled from DocumentLoader::processData(). Problem with current changes is that the media loading is starting too fast and it is getting cancelled. Where as with timer, the loading is started only after DocumentLoader::processData() so there is no problem. When i just change the cancellableandcreate task to just a timer it works fine. And the two failed test cases are related to media being loaded in an iframe.
On 2016/04/28 13:17:45, Srirama wrote: > On 2016/04/27 13:28:09, philipj_slow wrote: > > Thanks Srirama, from that and looking at the resource selection algorithm in > the > > spec I'm pretty sure that there is no stack overflow hiding here. The > > implementation isn't quite similar to the spec, but the problem would have > been > > if selectNextSourceChild was recursive or similar setup. > > > > So let's not worry more about that until such a problem appears in reality, > > which is hopefully never. > > > > If you address the other issues I think should be good for landing. > > Thanks philip. I was checking the two layout failure issues, and looks like the > request is getting cancalled. > This is getting cancelled from DocumentLoader::processData(). Problem with > current changes is that the media loading is starting too fast and it is getting > cancelled. Where as with timer, the loading is started only after > DocumentLoader::processData() so there is no problem. > When i just change the cancellableandcreate task to just a timer it works fine. > And the two failed test cases are related to media being loaded in an iframe. Hmm, so when a media resource is loaded directly in an iframe, it's like a top-level load of a media resource, i.e. one should end up with a MediaDocument. My understanding is that as soon as we've determined that it's a media resource, the connection is interrupted, a MediaDocument is created, and then the URL is set as the src of the video element within. But perhaps this understanding is just wrong? Is the *same* HTTP request reused, and canceling the original one also leaks over to the "new" (but same) request? After the stopFetching() call at the end of DocumentLoader::processData, how does that affect what happens in HTMLMediaElement? Do you end up in HTMLMediaElement::mediaEngineError?
On 2016/04/28 13:27:44, philipj_slow wrote: > On 2016/04/28 13:17:45, Srirama wrote: > > On 2016/04/27 13:28:09, philipj_slow wrote: > > > Thanks Srirama, from that and looking at the resource selection algorithm in > > the > > > spec I'm pretty sure that there is no stack overflow hiding here. The > > > implementation isn't quite similar to the spec, but the problem would have > > been > > > if selectNextSourceChild was recursive or similar setup. > > > > > > So let's not worry more about that until such a problem appears in reality, > > > which is hopefully never. > > > > > > If you address the other issues I think should be good for landing. > > > > Thanks philip. I was checking the two layout failure issues, and looks like > the > > request is getting cancalled. > > This is getting cancelled from DocumentLoader::processData(). Problem with > > current changes is that the media loading is starting too fast and it is > getting > > cancelled. Where as with timer, the loading is started only after > > DocumentLoader::processData() so there is no problem. > > When i just change the cancellableandcreate task to just a timer it works > fine. > > And the two failed test cases are related to media being loaded in an iframe. > > Hmm, so when a media resource is loaded directly in an iframe, it's like a > top-level load of a media resource, i.e. one should end up with a MediaDocument. > My understanding is that as soon as we've determined that it's a media resource, > the connection is interrupted, a MediaDocument is created, and then the URL is > set as the src of the video element within. > > But perhaps this understanding is just wrong? Is the *same* HTTP request reused, > and canceling the original one also leaks over to the "new" (but same) request? > > After the stopFetching() call at the end of DocumentLoader::processData, how > does that affect what happens in HTMLMediaElement? Do you end up in > HTMLMediaElement::mediaEngineError? Below is what happening as per my logs: DocumentLoader::ProcessData() BEGIN -> commitData() -> Realized it is a media resource, so creating media document with video element and add source element with src attribute -> invoke HTMLMediaElement load resource algorithm from sourceWasAdded() ->post cancellable task -> fired cancellable Task -> start media resource load -> DocumentLoader::ProcessData - END by cancelling requests. So this is cancelling event the current media resource and we are getting setNetworkState() with format error (medialoadingfailed() and scheduleerrorevent()). Not sure if both are using same ResourceFetcher/ResourceLoader, but in logs i can see resource_loader.cc:OnResponseStarted for both the requests. One doubt i have here is, doesn't cancellabletask wait for DocumentLoader::ProcessData() to finish before it gets fired?
On 2016/04/28 14:02:56, Srirama wrote: > On 2016/04/28 13:27:44, philipj_slow wrote: > > On 2016/04/28 13:17:45, Srirama wrote: > > > On 2016/04/27 13:28:09, philipj_slow wrote: > > > > Thanks Srirama, from that and looking at the resource selection algorithm > in > > > the > > > > spec I'm pretty sure that there is no stack overflow hiding here. The > > > > implementation isn't quite similar to the spec, but the problem would have > > > been > > > > if selectNextSourceChild was recursive or similar setup. > > > > > > > > So let's not worry more about that until such a problem appears in > reality, > > > > which is hopefully never. > > > > > > > > If you address the other issues I think should be good for landing. > > > > > > Thanks philip. I was checking the two layout failure issues, and looks like > > the > > > request is getting cancalled. > > > This is getting cancelled from DocumentLoader::processData(). Problem with > > > current changes is that the media loading is starting too fast and it is > > getting > > > cancelled. Where as with timer, the loading is started only after > > > DocumentLoader::processData() so there is no problem. > > > When i just change the cancellableandcreate task to just a timer it works > > fine. > > > And the two failed test cases are related to media being loaded in an > iframe. > > > > Hmm, so when a media resource is loaded directly in an iframe, it's like a > > top-level load of a media resource, i.e. one should end up with a > MediaDocument. > > My understanding is that as soon as we've determined that it's a media > resource, > > the connection is interrupted, a MediaDocument is created, and then the URL is > > set as the src of the video element within. > > > > But perhaps this understanding is just wrong? Is the *same* HTTP request > reused, > > and canceling the original one also leaks over to the "new" (but same) > request? > > > > After the stopFetching() call at the end of DocumentLoader::processData, how > > does that affect what happens in HTMLMediaElement? Do you end up in > > HTMLMediaElement::mediaEngineError? > > Below is what happening as per my logs: > DocumentLoader::ProcessData() BEGIN -> commitData() -> Realized it is a media > resource, so creating media document with video element and add source element > with src attribute -> invoke HTMLMediaElement load resource algorithm from > sourceWasAdded() ->post cancellable task -> fired cancellable Task -> start > media resource load -> DocumentLoader::ProcessData - END by cancelling requests. > So this is cancelling event the current media resource and we are getting > setNetworkState() with format error (medialoadingfailed() and > scheduleerrorevent()). > Not sure if both are using same ResourceFetcher/ResourceLoader, but in logs i > can see resource_loader.cc:OnResponseStarted for both the requests. Have you looked into the details of what it is that triggers the setNetworkState with NetworkStateFormatError? I guess it's from a call to networkStateChanged() from the media player, but where did it originate? It's a little bit surprising that the two resource fetches should affect each other, but I'm no networking expert. > One doubt i have here is, doesn't cancellabletask wait for > DocumentLoader::ProcessData() to finish before it gets fired? No, it's posted as a microtask so it will run at the next microtask checkpoint. I suggest seeking advice from someone who understands the networking stack better, and especially its usage in Blink. It makes sense that the order of the task relative to the stopFetching() call can have changed, but to me it doesn't make sense that the two should be connected in any way even though it's the same URL.
On 2016/04/29 08:23:59, philipj_slow wrote: > On 2016/04/28 14:02:56, Srirama wrote: > > On 2016/04/28 13:27:44, philipj_slow wrote: > > > On 2016/04/28 13:17:45, Srirama wrote: > > > > On 2016/04/27 13:28:09, philipj_slow wrote: > > > > > Thanks Srirama, from that and looking at the resource selection > algorithm > > in > > > > the > > > > > spec I'm pretty sure that there is no stack overflow hiding here. The > > > > > implementation isn't quite similar to the spec, but the problem would > have > > > > been > > > > > if selectNextSourceChild was recursive or similar setup. > > > > > > > > > > So let's not worry more about that until such a problem appears in > > reality, > > > > > which is hopefully never. > > > > > > > > > > If you address the other issues I think should be good for landing. > > > > > > > > Thanks philip. I was checking the two layout failure issues, and looks > like > > > the > > > > request is getting cancalled. > > > > This is getting cancelled from DocumentLoader::processData(). Problem with > > > > current changes is that the media loading is starting too fast and it is > > > getting > > > > cancelled. Where as with timer, the loading is started only after > > > > DocumentLoader::processData() so there is no problem. > > > > When i just change the cancellableandcreate task to just a timer it works > > > fine. > > > > And the two failed test cases are related to media being loaded in an > > iframe. > > > > > > Hmm, so when a media resource is loaded directly in an iframe, it's like a > > > top-level load of a media resource, i.e. one should end up with a > > MediaDocument. > > > My understanding is that as soon as we've determined that it's a media > > resource, > > > the connection is interrupted, a MediaDocument is created, and then the URL > is > > > set as the src of the video element within. > > > > > > But perhaps this understanding is just wrong? Is the *same* HTTP request > > reused, > > > and canceling the original one also leaks over to the "new" (but same) > > request? > > > > > > After the stopFetching() call at the end of DocumentLoader::processData, how > > > does that affect what happens in HTMLMediaElement? Do you end up in > > > HTMLMediaElement::mediaEngineError? > > > > Below is what happening as per my logs: > > DocumentLoader::ProcessData() BEGIN -> commitData() -> Realized it is a media > > resource, so creating media document with video element and add source element > > with src attribute -> invoke HTMLMediaElement load resource algorithm from > > sourceWasAdded() ->post cancellable task -> fired cancellable Task -> start > > media resource load -> DocumentLoader::ProcessData - END by cancelling > requests. > > So this is cancelling event the current media resource and we are getting > > setNetworkState() with format error (medialoadingfailed() and > > scheduleerrorevent()). > > Not sure if both are using same ResourceFetcher/ResourceLoader, but in logs i > > can see resource_loader.cc:OnResponseStarted for both the requests. > > Have you looked into the details of what it is that triggers the setNetworkState > with NetworkStateFormatError? I guess it's from a call to networkStateChanged() > from the media player, but where did it originate? > Yes, it is networkStateChanged() with NetworkStateFormatError BufferedDataSource::StartCallback() -> WebMediaPlayerImpl::DataSourceInitialized() because BufferedResourceLoader status is not OK because of load cancel. > It's a little bit surprising that the two resource fetches should affect each > other, but I'm no networking expert. > Is it really to do with whether two resource fetches are related or not? I think when we identify that it is a media document, we are trying to cancel all the outstanding requests pending. So i feel it is not about whether two requests are related or not. Ideally media document should not have any inline requests to be cancelled. > > One doubt i have here is, doesn't cancellabletask wait for > > DocumentLoader::ProcessData() to finish before it gets fired? > > No, it's posted as a microtask so it will run at the next microtask checkpoint. > > I suggest seeking advice from someone who understands the networking stack > better, and especially its usage in Blink. It makes sense that the order of the > task relative to the stopFetching() call can have changed, but to me it doesn't > make sense that the two should be connected in any way even though it's the same > URL. How about cancelling the requests as soon as we identify it is a mediadocument and before constructing the media document tree? Probably i will check with network experts as you said.
On 2016/04/29 09:44:33, Srirama wrote: > On 2016/04/29 08:23:59, philipj_slow wrote: > > On 2016/04/28 14:02:56, Srirama wrote: > > > On 2016/04/28 13:27:44, philipj_slow wrote: > > > > On 2016/04/28 13:17:45, Srirama wrote: > > > > > On 2016/04/27 13:28:09, philipj_slow wrote: > > > > > > Thanks Srirama, from that and looking at the resource selection > > algorithm > > > in > > > > > the > > > > > > spec I'm pretty sure that there is no stack overflow hiding here. The > > > > > > implementation isn't quite similar to the spec, but the problem would > > have > > > > > been > > > > > > if selectNextSourceChild was recursive or similar setup. > > > > > > > > > > > > So let's not worry more about that until such a problem appears in > > > reality, > > > > > > which is hopefully never. > > > > > > > > > > > > If you address the other issues I think should be good for landing. > > > > > > > > > > Thanks philip. I was checking the two layout failure issues, and looks > > like > > > > the > > > > > request is getting cancalled. > > > > > This is getting cancelled from DocumentLoader::processData(). Problem > with > > > > > current changes is that the media loading is starting too fast and it is > > > > getting > > > > > cancelled. Where as with timer, the loading is started only after > > > > > DocumentLoader::processData() so there is no problem. > > > > > When i just change the cancellableandcreate task to just a timer it > works > > > > fine. > > > > > And the two failed test cases are related to media being loaded in an > > > iframe. > > > > > > > > Hmm, so when a media resource is loaded directly in an iframe, it's like a > > > > top-level load of a media resource, i.e. one should end up with a > > > MediaDocument. > > > > My understanding is that as soon as we've determined that it's a media > > > resource, > > > > the connection is interrupted, a MediaDocument is created, and then the > URL > > is > > > > set as the src of the video element within. > > > > > > > > But perhaps this understanding is just wrong? Is the *same* HTTP request > > > reused, > > > > and canceling the original one also leaks over to the "new" (but same) > > > request? > > > > > > > > After the stopFetching() call at the end of DocumentLoader::processData, > how > > > > does that affect what happens in HTMLMediaElement? Do you end up in > > > > HTMLMediaElement::mediaEngineError? > > > > > > Below is what happening as per my logs: > > > DocumentLoader::ProcessData() BEGIN -> commitData() -> Realized it is a > media > > > resource, so creating media document with video element and add source > element > > > with src attribute -> invoke HTMLMediaElement load resource algorithm from > > > sourceWasAdded() ->post cancellable task -> fired cancellable Task -> start > > > media resource load -> DocumentLoader::ProcessData - END by cancelling > > requests. > > > So this is cancelling event the current media resource and we are getting > > > setNetworkState() with format error (medialoadingfailed() and > > > scheduleerrorevent()). > > > Not sure if both are using same ResourceFetcher/ResourceLoader, but in logs > i > > > can see resource_loader.cc:OnResponseStarted for both the requests. > > > > Have you looked into the details of what it is that triggers the > setNetworkState > > with NetworkStateFormatError? I guess it's from a call to > networkStateChanged() > > from the media player, but where did it originate? > > > Yes, it is networkStateChanged() with NetworkStateFormatError > BufferedDataSource::StartCallback() -> > WebMediaPlayerImpl::DataSourceInitialized() because BufferedResourceLoader > status is not OK because of load cancel. > > > It's a little bit surprising that the two resource fetches should affect each > > other, but I'm no networking expert. > > > Is it really to do with whether two resource fetches are related or not? I think > when we identify that it is a media document, we are trying to cancel all the > outstanding requests pending. So i feel it is not about whether two requests are > related or not. Ideally media document should not have any inline requests to be > cancelled. In principle each fetch should be independent, but maybe the problem is that the way of canceling the request is pretending that there was an error. For a real error it would make sense for the error to propagate to all fetches, so perhaps this is a problem of how canceling is implemented. > > > One doubt i have here is, doesn't cancellabletask wait for > > > DocumentLoader::ProcessData() to finish before it gets fired? > > > > No, it's posted as a microtask so it will run at the next microtask > checkpoint. > > > > I suggest seeking advice from someone who understands the networking stack > > better, and especially its usage in Blink. It makes sense that the order of > the > > task relative to the stopFetching() call can have changed, but to me it > doesn't > > make sense that the two should be connected in any way even though it's the > same > > URL. > How about cancelling the requests as soon as we identify it is a mediadocument > and before constructing the media document tree? > Probably i will check with network experts as you said. You might be able to change the order of things or delay creation of the MediaDocument so that this problem goes away, but it would be good to consult a networking expert. This is my last day and I won't be able to check back in on this, but I wish you the best of luck with landing this, it's a very nice improvement I think.
srirama.m@samsung.com changed reviewers: + japhet@chromium.org
@Nate, could you please give your inputs or suggestions here from blink loader perspective. Right now HTMLMediaElement uses a plain timer task and that i am trying to change it to microtask as per spec. Now the problem i am facing here is, when i load a video as an iframe document (iframe src=video). What is happening previously is we get response for iframe request, we realize it is a media and create mediadocument, set the 'src' to video element which posts a timer to load it. And we cancel all the pending requests(DocumentLoader::prcoessData). Then timer fires and starts loading video as part of video element. But with new changes, everything is same except the microtask fires sooner and start loading video, before we finish DocumentLoader::prcoessData, so when we cancel all the pending requests in DocumentLoader::prcoessData, we end up cancelling the new video request also. Please see comments #44 onwards for more details.
srirama.m@samsung.com changed reviewers: + mkwst@chromium.org
+Mike West. @Mike, can you please give your inputs here? Refer to the comment #51 for the issue details.
On 2016/05/10 06:49:28, Srirama wrote: > +Mike West. > @Mike, can you please give your inputs here? Refer to the comment #51 for the > issue details. @fs, any suggestion, looks like Nate and Mike are not active right now.
On 2016/05/16 at 10:26:44, srirama.m wrote: > On 2016/05/10 06:49:28, Srirama wrote: > > +Mike West. > > @Mike, can you please give your inputs here? Refer to the comment #51 for the > > issue details. > > @fs, any suggestion, looks like Nate and Mike are not active right now. What does the stack look like when the task (the one that initiates the "new" load) runs? (I.e what is flushing the microtask queue?)
On 2016/05/16 10:58:25, fs wrote:
> On 2016/05/16 at 10:26:44, srirama.m wrote:
> > On 2016/05/10 06:49:28, Srirama wrote:
> > > +Mike West.
> > > @Mike, can you please give your inputs here? Refer to the comment #51 for
> the
> > > issue details.
> >
> > @fs, any suggestion, looks like Nate and Mike are not active right now.
>
> What does the stack look like when the task (the one that initiates the "new"
> load) runs? (I.e what is flushing the microtask queue?)
Below is the callstack and there is a Microtask::performCheckpoint() in
Document::finishedParsing() with a FIXME as well.
*************************
#0 blink::HTMLMediaElement::continueResourceSelectionAlgorithm() () at
../../third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:813
#1 0x00007fbac936e80e in
_ZN3WTF15FunctionWrapperIMN5blink16HTMLMediaElementEFvvEEclIJEEEvRKNS_7WeakPtrIS2_EEDpOT_
()
at ../../third_party/WebKit/Source/wtf/Functional.h:173
#2 0x00007fbac936e762 in
_ZN3WTF21PartBoundFunctionImplILNS_22FunctionThreadAffinityE1ESt5tupleIJON5blink36CrossThreadWeakPersistentThisPointerINS3_16HTMLMediaElementEEEEENS_15FunctionWrapperIMS5_FvvEEEJEE12callInternalIJLm0EEJEEEvRKN4base13IndexSequenceIJXspT_EEEEDpOT0_
() at ../../third_party/WebKit/Source/wtf/Functional.h:318
#3 0x00007fbac936e2c9 in
_ZN3WTF21PartBoundFunctionImplILNS_22FunctionThreadAffinityE1ESt5tupleIJON5blink36CrossThreadWeakPersistentThisPointerINS3_16HTMLMediaElementEEEEENS_15FunctionWrapperIMS5_FvvEEEJEEclEv
() at ../../third_party/WebKit/Source/wtf/Functional.h:309
#4 0x00007fbae122367a in blink::CancellableTaskFactory::CancellableTask::run()
() at
../../third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.cpp:27
#5 0x00007fbac7e6ecbc in blink::microtaskFunctionCallback(void*) () at
../../third_party/WebKit/Source/bindings/core/v8/Microtask.cpp:48
#6 0x00007fbad354d809 in v8::internal::Isolate::RunMicrotasksInternal() () at
../../v8/src/isolate.cc:2801
#7 0x00007fbad354d0e3 in v8::internal::Isolate::RunMicrotasks() () at
../../v8/src/isolate.cc:2785
#8 0x00007fbac7e6ec0c in blink::Microtask::performCheckpoint(v8::Isolate*) ()
at ../../third_party/WebKit/Source/bindings/core/v8/Microtask.cpp:42
#9 0x00007fbac8f78a46 in blink::Document::finishedParsing() () at
../../third_party/WebKit/Source/core/dom/Document.cpp:4725
#10 0x00007fbac8d5508b in blink::RawDataDocumentParser::finish() () at
../../third_party/WebKit/Source/core/dom/RawDataDocumentParser.h:44
#11 0x00007fbac93be61b in blink::MediaDocumentParser::appendBytes(char const*,
unsigned long) () at
../../third_party/WebKit/Source/core/html/MediaDocument.cpp:230
#12 0x00007fbac8d1ac79 in blink::DocumentWriter::addData(char const*, unsigned
long) () at ../../third_party/WebKit/Source/core/loader/DocumentWriter.cpp:93
#13 0x00007fbac8d06bed in blink::DocumentLoader::commitData(char const*,
unsigned long) () at
../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:472
#14 0x00007fbac8d086c9 in blink::DocumentLoader::processData(char const*,
unsigned long) () at
../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:521
#15 0x00007fbac8d08552 in blink::DocumentLoader::dataReceived(blink::Resource*,
char const*, unsigned long) ()
at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:496
On 2016/05/16 at 11:38:32, srirama.m wrote: > On 2016/05/16 10:58:25, fs wrote: > > On 2016/05/16 at 10:26:44, srirama.m wrote: > > > On 2016/05/10 06:49:28, Srirama wrote: > > > > +Mike West. > > > > @Mike, can you please give your inputs here? Refer to the comment #51 for > > the > > > > issue details. > > > > > > @fs, any suggestion, looks like Nate and Mike are not active right now. > > > > What does the stack look like when the task (the one that initiates the "new" > > load) runs? (I.e what is flushing the microtask queue?) > > Below is the callstack and there is a Microtask::performCheckpoint() in Document::finishedParsing() with a FIXME as well. > ************************* > #0 blink::HTMLMediaElement::continueResourceSelectionAlgorithm() () at ../../third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:813 > #1 0x00007fbac936e80e in _ZN3WTF15FunctionWrapperIMN5blink16HTMLMediaElementEFvvEEclIJEEEvRKNS_7WeakPtrIS2_EEDpOT_ () > at ../../third_party/WebKit/Source/wtf/Functional.h:173 > #2 0x00007fbac936e762 in _ZN3WTF21PartBoundFunctionImplILNS_22FunctionThreadAffinityE1ESt5tupleIJON5blink36CrossThreadWeakPersistentThisPointerINS3_16HTMLMediaElementEEEEENS_15FunctionWrapperIMS5_FvvEEEJEE12callInternalIJLm0EEJEEEvRKN4base13IndexSequenceIJXspT_EEEEDpOT0_ () at ../../third_party/WebKit/Source/wtf/Functional.h:318 > #3 0x00007fbac936e2c9 in _ZN3WTF21PartBoundFunctionImplILNS_22FunctionThreadAffinityE1ESt5tupleIJON5blink36CrossThreadWeakPersistentThisPointerINS3_16HTMLMediaElementEEEEENS_15FunctionWrapperIMS5_FvvEEEJEEclEv () at ../../third_party/WebKit/Source/wtf/Functional.h:309 > #4 0x00007fbae122367a in blink::CancellableTaskFactory::CancellableTask::run() () at ../../third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.cpp:27 > #5 0x00007fbac7e6ecbc in blink::microtaskFunctionCallback(void*) () at ../../third_party/WebKit/Source/bindings/core/v8/Microtask.cpp:48 > #6 0x00007fbad354d809 in v8::internal::Isolate::RunMicrotasksInternal() () at ../../v8/src/isolate.cc:2801 > #7 0x00007fbad354d0e3 in v8::internal::Isolate::RunMicrotasks() () at ../../v8/src/isolate.cc:2785 > #8 0x00007fbac7e6ec0c in blink::Microtask::performCheckpoint(v8::Isolate*) () at ../../third_party/WebKit/Source/bindings/core/v8/Microtask.cpp:42 > #9 0x00007fbac8f78a46 in blink::Document::finishedParsing() () at ../../third_party/WebKit/Source/core/dom/Document.cpp:4725 > #10 0x00007fbac8d5508b in blink::RawDataDocumentParser::finish() () at ../../third_party/WebKit/Source/core/dom/RawDataDocumentParser.h:44 > #11 0x00007fbac93be61b in blink::MediaDocumentParser::appendBytes(char const*, unsigned long) () at ../../third_party/WebKit/Source/core/html/MediaDocument.cpp:230 > #12 0x00007fbac8d1ac79 in blink::DocumentWriter::addData(char const*, unsigned long) () at ../../third_party/WebKit/Source/core/loader/DocumentWriter.cpp:93 > #13 0x00007fbac8d06bed in blink::DocumentLoader::commitData(char const*, unsigned long) () at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:472 > #14 0x00007fbac8d086c9 in blink::DocumentLoader::processData(char const*, unsigned long) () at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:521 > #15 0x00007fbac8d08552 in blink::DocumentLoader::dataReceived(blink::Resource*, char const*, unsigned long) () > at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:496 Thanks. Since there seems to be some traction on the related bug (crbug.com/425790) again, it might be reasonable to "wait and see" for a bit. Any form of explicit deferral will likely be hacky anyway (but should certainly be possible.)
On 2016/05/16 12:10:49, fs wrote: > On 2016/05/16 at 11:38:32, srirama.m wrote: > > On 2016/05/16 10:58:25, fs wrote: > > > On 2016/05/16 at 10:26:44, srirama.m wrote: > > > > On 2016/05/10 06:49:28, Srirama wrote: > > > > > +Mike West. > > > > > @Mike, can you please give your inputs here? Refer to the comment #51 > for > > > the > > > > > issue details. > > > > > > > > @fs, any suggestion, looks like Nate and Mike are not active right now. > > > > > > What does the stack look like when the task (the one that initiates the > "new" > > > load) runs? (I.e what is flushing the microtask queue?) > > > > Below is the callstack and there is a Microtask::performCheckpoint() in > Document::finishedParsing() with a FIXME as well. > > ************************* > > #0 blink::HTMLMediaElement::continueResourceSelectionAlgorithm() () at > ../../third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:813 > > #1 0x00007fbac936e80e in > _ZN3WTF15FunctionWrapperIMN5blink16HTMLMediaElementEFvvEEclIJEEEvRKNS_7WeakPtrIS2_EEDpOT_ > () > > at ../../third_party/WebKit/Source/wtf/Functional.h:173 > > #2 0x00007fbac936e762 in > _ZN3WTF21PartBoundFunctionImplILNS_22FunctionThreadAffinityE1ESt5tupleIJON5blink36CrossThreadWeakPersistentThisPointerINS3_16HTMLMediaElementEEEEENS_15FunctionWrapperIMS5_FvvEEEJEE12callInternalIJLm0EEJEEEvRKN4base13IndexSequenceIJXspT_EEEEDpOT0_ > () at ../../third_party/WebKit/Source/wtf/Functional.h:318 > > #3 0x00007fbac936e2c9 in > _ZN3WTF21PartBoundFunctionImplILNS_22FunctionThreadAffinityE1ESt5tupleIJON5blink36CrossThreadWeakPersistentThisPointerINS3_16HTMLMediaElementEEEEENS_15FunctionWrapperIMS5_FvvEEEJEEclEv > () at ../../third_party/WebKit/Source/wtf/Functional.h:309 > > #4 0x00007fbae122367a in > blink::CancellableTaskFactory::CancellableTask::run() () at > ../../third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.cpp:27 > > #5 0x00007fbac7e6ecbc in blink::microtaskFunctionCallback(void*) () at > ../../third_party/WebKit/Source/bindings/core/v8/Microtask.cpp:48 > > #6 0x00007fbad354d809 in v8::internal::Isolate::RunMicrotasksInternal() () at > ../../v8/src/isolate.cc:2801 > > #7 0x00007fbad354d0e3 in v8::internal::Isolate::RunMicrotasks() () at > ../../v8/src/isolate.cc:2785 > > #8 0x00007fbac7e6ec0c in blink::Microtask::performCheckpoint(v8::Isolate*) () > at ../../third_party/WebKit/Source/bindings/core/v8/Microtask.cpp:42 > > #9 0x00007fbac8f78a46 in blink::Document::finishedParsing() () at > ../../third_party/WebKit/Source/core/dom/Document.cpp:4725 > > #10 0x00007fbac8d5508b in blink::RawDataDocumentParser::finish() () at > ../../third_party/WebKit/Source/core/dom/RawDataDocumentParser.h:44 > > #11 0x00007fbac93be61b in blink::MediaDocumentParser::appendBytes(char const*, > unsigned long) () at > ../../third_party/WebKit/Source/core/html/MediaDocument.cpp:230 > > #12 0x00007fbac8d1ac79 in blink::DocumentWriter::addData(char const*, unsigned > long) () at ../../third_party/WebKit/Source/core/loader/DocumentWriter.cpp:93 > > #13 0x00007fbac8d06bed in blink::DocumentLoader::commitData(char const*, > unsigned long) () at > ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:472 > > #14 0x00007fbac8d086c9 in blink::DocumentLoader::processData(char const*, > unsigned long) () at > ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:521 > > #15 0x00007fbac8d08552 in > blink::DocumentLoader::dataReceived(blink::Resource*, char const*, unsigned > long) () > > at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:496 > > Thanks. Since there seems to be some traction on the related bug > (crbug.com/425790) again, it might be reasonable to "wait and see" for a bit. > Any form of explicit deferral will likely be hacky anyway (but should certainly > be possible.) Thanks @fs, Agreed, will wait for that bug and meanwhile, i will try to understand and debug more from my side if i can.
foolip@chromium.org changed reviewers: + foolip@chromium.org - philipj@opera.com
Patchset #6 (id:160001) has been deleted
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/v2/patch-status/codereview.chromium.or...
@foolip, @fs, PTAL. To give you the context on why it was stalled, some of the video test cases are failing because microtask to load video is getting fired due to microtask check point (Microtask::performCheckpoint) done in Document::finishedParsing() and this new video load is getting cancelled wrongly from DocumentLoader::processData(). Changes from the previous patchset. 1) Rebase 2) Used Platform::current()->currentThread()->getWebTaskRunner()->postTask() instead of enqueueMicrotask and i think this will solve the problem of micro task check point. Ran the media tests locally and those previous failures seems to be gone. Hopefully CQ dry run will be successful as well.
On 2016/08/31 at 14:58:52, srirama.m wrote: > @foolip, @fs, PTAL. > > To give you the context on why it was stalled, some of the video test cases are failing because microtask to load video is getting fired due to microtask check point (Microtask::performCheckpoint) done in Document::finishedParsing() and this new video load is getting cancelled wrongly from DocumentLoader::processData(). > > Changes from the previous patchset. > 1) Rebase > 2) Used Platform::current()->currentThread()->getWebTaskRunner()->postTask() instead of enqueueMicrotask and i think this will solve the problem of micro task check point. > Ran the media tests locally and those previous failures seems to be gone. > Hopefully CQ dry run will be successful as well. This is not wildly different from what was there before (different taskrunner), but maybe it counts as a progression. I'll let foolip decide. Could we make a test that will notice when the performCheckpoint call in Document::finishedParsing goes away? (So we can change this too then.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/31 15:17:24, fs wrote: > On 2016/08/31 at 14:58:52, srirama.m wrote: > > @foolip, @fs, PTAL. > > > > To give you the context on why it was stalled, some of the video test cases > are failing because microtask to load video is getting fired due to microtask > check point (Microtask::performCheckpoint) done in Document::finishedParsing() > and this new video load is getting cancelled wrongly from > DocumentLoader::processData(). > > > > Changes from the previous patchset. > > 1) Rebase > > 2) Used Platform::current()->currentThread()->getWebTaskRunner()->postTask() > instead of enqueueMicrotask and i think this will solve the problem of micro > task check point. > > Ran the media tests locally and those previous failures seems to be gone. > > Hopefully CQ dry run will be successful as well. > > This is not wildly different from what was there before (different taskrunner), > but maybe it counts as a progression. I'll let foolip decide. Could we make a > test that will notice when the performCheckpoint call in > Document::finishedParsing goes away? (So we can change this too then.) Does Platform::current()->currentThread()->getWebTaskRunner()->postTask() amount to "post a task" in spec terms, how is it different? Getting microtask checkpoint timing is important and the cause of failing tests in web-platform-tests, so I'm skeptical of changing the timing in any observable way other than aligning with that. Per spec, is it correct that there should be a microtask checkpoint at Document::finishedParsing()?
On 2016/09/01 12:36:29, foolip wrote: > On 2016/08/31 15:17:24, fs wrote: > > On 2016/08/31 at 14:58:52, srirama.m wrote: > > > @foolip, @fs, PTAL. > > > > > > To give you the context on why it was stalled, some of the video test cases > > are failing because microtask to load video is getting fired due to microtask > > check point (Microtask::performCheckpoint) done in Document::finishedParsing() > > and this new video load is getting cancelled wrongly from > > DocumentLoader::processData(). > > > > > > Changes from the previous patchset. > > > 1) Rebase > > > 2) Used Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > instead of enqueueMicrotask and i think this will solve the problem of micro > > task check point. > > > Ran the media tests locally and those previous failures seems to be gone. > > > Hopefully CQ dry run will be successful as well. > > > > This is not wildly different from what was there before (different > taskrunner), > > but maybe it counts as a progression. I'll let foolip decide. Could we make a > > test that will notice when the performCheckpoint call in > > Document::finishedParsing goes away? (So we can change this too then.) > > Does Platform::current()->currentThread()->getWebTaskRunner()->postTask() amount > to "post a task" in spec terms, how is it different? > > Getting microtask checkpoint timing is important and the cause of failing tests > in web-platform-tests, so I'm skeptical of changing the timing in any observable > way other than aligning with that. > > Per spec, is it correct that there should be a microtask checkpoint at > Document::finishedParsing()? You may want to consider using the task runner that corresponds to the one in the spec through here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskR...
On 2016/09/01 at 12:42:56, skyostil wrote: > On 2016/09/01 12:36:29, foolip wrote: > > On 2016/08/31 15:17:24, fs wrote: > > > On 2016/08/31 at 14:58:52, srirama.m wrote: > > > > @foolip, @fs, PTAL. > > > > > > > > To give you the context on why it was stalled, some of the video test cases > > > are failing because microtask to load video is getting fired due to microtask > > > check point (Microtask::performCheckpoint) done in Document::finishedParsing() > > > and this new video load is getting cancelled wrongly from > > > DocumentLoader::processData(). > > > > > > > > Changes from the previous patchset. > > > > 1) Rebase > > > > 2) Used Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > > instead of enqueueMicrotask and i think this will solve the problem of micro > > > task check point. > > > > Ran the media tests locally and those previous failures seems to be gone. > > > > Hopefully CQ dry run will be successful as well. > > > > > > This is not wildly different from what was there before (different > > taskrunner), > > > but maybe it counts as a progression. I'll let foolip decide. Could we make a > > > test that will notice when the performCheckpoint call in > > > Document::finishedParsing goes away? (So we can change this too then.) > > > > Does Platform::current()->currentThread()->getWebTaskRunner()->postTask() amount > > to "post a task" in spec terms, how is it different? I think it's a close enough approximation. Compared to before it uses a different task runner though (timer vs. "generic".) > > Getting microtask checkpoint timing is important and the cause of failing tests > > in web-platform-tests, so I'm skeptical of changing the timing in any observable > > way other than aligning with that. > > > > Per spec, is it correct that there should be a microtask checkpoint at > > Document::finishedParsing()? IIUC, the reason for that checkpoint is because DOMContentLoaded isn't posted as a task, so this checkpoint serves to emulate that (i.e step 6 for that would-be task.) > You may want to consider using the task runner that corresponds to the one in the spec through here: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskR... Since the mappings haven't diverged much (yet), that might work. Would need to look at other cases too though to avoid affecting the ordering.
On 2016/09/01 13:08:48, fs wrote: > On 2016/09/01 at 12:42:56, skyostil wrote: > > On 2016/09/01 12:36:29, foolip wrote: > > > On 2016/08/31 15:17:24, fs wrote: > > > > On 2016/08/31 at 14:58:52, srirama.m wrote: > > > > > @foolip, @fs, PTAL. > > > > > > > > > > To give you the context on why it was stalled, some of the video test > cases > > > > are failing because microtask to load video is getting fired due to > microtask > > > > check point (Microtask::performCheckpoint) done in > Document::finishedParsing() > > > > and this new video load is getting cancelled wrongly from > > > > DocumentLoader::processData(). > > > > > > > > > > Changes from the previous patchset. > > > > > 1) Rebase > > > > > 2) Used > Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > > > instead of enqueueMicrotask and i think this will solve the problem of > micro > > > > task check point. > > > > > Ran the media tests locally and those previous failures seems to be > gone. > > > > > Hopefully CQ dry run will be successful as well. > > > > > > > > This is not wildly different from what was there before (different > > > taskrunner), > > > > but maybe it counts as a progression. I'll let foolip decide. Could we > make a > > > > test that will notice when the performCheckpoint call in > > > > Document::finishedParsing goes away? (So we can change this too then.) > > > > > > Does Platform::current()->currentThread()->getWebTaskRunner()->postTask() > amount > > > to "post a task" in spec terms, how is it different? > > I think it's a close enough approximation. Compared to before it uses a > different task runner though (timer vs. "generic".) > > > > Getting microtask checkpoint timing is important and the cause of failing > tests > > > in web-platform-tests, so I'm skeptical of changing the timing in any > observable > > > way other than aligning with that. > > > > > > Per spec, is it correct that there should be a microtask checkpoint at > > > Document::finishedParsing()? > > IIUC, the reason for that checkpoint is because DOMContentLoaded isn't posted as > a task, so this checkpoint serves to emulate that (i.e step 6 for that would-be > task.) > > > You may want to consider using the task runner that corresponds to the one in > the spec through here: > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskR... > > Since the mappings haven't diverged much (yet), that might work. Would need to > look at other cases too though to avoid affecting the ordering. Probably a TODO for now to revisit it once the mappings are more clear in TaskRunnerHelper::get()? And regarding the test, do we need to write a separate test for this? I think WPT has good number of tests for this /html/semantics/embedded-content/media-elements/loading-the-media-resource/resource-selection*, so @fs WDYT?
On 2016/09/02 at 07:47:56, srirama.m wrote: > On 2016/09/01 13:08:48, fs wrote: > > On 2016/09/01 at 12:42:56, skyostil wrote: > > > On 2016/09/01 12:36:29, foolip wrote: > > > > On 2016/08/31 15:17:24, fs wrote: > > > > > On 2016/08/31 at 14:58:52, srirama.m wrote: > > > > > > @foolip, @fs, PTAL. > > > > > > > > > > > > To give you the context on why it was stalled, some of the video test > > cases > > > > > are failing because microtask to load video is getting fired due to > > microtask > > > > > check point (Microtask::performCheckpoint) done in > > Document::finishedParsing() > > > > > and this new video load is getting cancelled wrongly from > > > > > DocumentLoader::processData(). > > > > > > > > > > > > Changes from the previous patchset. > > > > > > 1) Rebase > > > > > > 2) Used > > Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > > > > instead of enqueueMicrotask and i think this will solve the problem of > > micro > > > > > task check point. > > > > > > Ran the media tests locally and those previous failures seems to be > > gone. > > > > > > Hopefully CQ dry run will be successful as well. > > > > > > > > > > This is not wildly different from what was there before (different > > > > taskrunner), > > > > > but maybe it counts as a progression. I'll let foolip decide. Could we > > make a > > > > > test that will notice when the performCheckpoint call in > > > > > Document::finishedParsing goes away? (So we can change this too then.) > > > > > > > > Does Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > amount > > > > to "post a task" in spec terms, how is it different? > > > > I think it's a close enough approximation. Compared to before it uses a > > different task runner though (timer vs. "generic".) > > > > > > Getting microtask checkpoint timing is important and the cause of failing > > tests > > > > in web-platform-tests, so I'm skeptical of changing the timing in any > > observable > > > > way other than aligning with that. > > > > > > > > Per spec, is it correct that there should be a microtask checkpoint at > > > > Document::finishedParsing()? > > > > IIUC, the reason for that checkpoint is because DOMContentLoaded isn't posted as > > a task, so this checkpoint serves to emulate that (i.e step 6 for that would-be > > task.) > > > > > You may want to consider using the task runner that corresponds to the one in > > the spec through here: > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskR... > > > > Since the mappings haven't diverged much (yet), that might work. Would need to > > look at other cases too though to avoid affecting the ordering. > > Probably a TODO for now to revisit it once the mappings are more clear in TaskRunnerHelper::get()? It's not so much about the mapping itself, but rather the actual ordering of the tasks. From that PoV it might be better to go through the code and note which task sources should be used for each postTask (and "non-timer Timer"). > And regarding the test, do we need to write a separate test for this? I think WPT has good number of tests for this /html/semantics/embedded-content/media-elements/loading-the-media-resource/resource-selection*, so @fs WDYT? If they cover microtask semantics well enough then I'm fine with that.
On 2016/09/02 08:08:51, fs wrote: > On 2016/09/02 at 07:47:56, srirama.m wrote: > > On 2016/09/01 13:08:48, fs wrote: > > > On 2016/09/01 at 12:42:56, skyostil wrote: > > > > On 2016/09/01 12:36:29, foolip wrote: > > > > > On 2016/08/31 15:17:24, fs wrote: > > > > > > On 2016/08/31 at 14:58:52, srirama.m wrote: > > > > > > > @foolip, @fs, PTAL. > > > > > > > > > > > > > > To give you the context on why it was stalled, some of the video > test > > > cases > > > > > > are failing because microtask to load video is getting fired due to > > > microtask > > > > > > check point (Microtask::performCheckpoint) done in > > > Document::finishedParsing() > > > > > > and this new video load is getting cancelled wrongly from > > > > > > DocumentLoader::processData(). > > > > > > > > > > > > > > Changes from the previous patchset. > > > > > > > 1) Rebase > > > > > > > 2) Used > > > Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > > > > > instead of enqueueMicrotask and i think this will solve the problem of > > > micro > > > > > > task check point. > > > > > > > Ran the media tests locally and those previous failures seems to be > > > gone. > > > > > > > Hopefully CQ dry run will be successful as well. > > > > > > > > > > > > This is not wildly different from what was there before (different > > > > > taskrunner), > > > > > > but maybe it counts as a progression. I'll let foolip decide. Could we > > > make a > > > > > > test that will notice when the performCheckpoint call in > > > > > > Document::finishedParsing goes away? (So we can change this too then.) > > > > > > > > > > Does > Platform::current()->currentThread()->getWebTaskRunner()->postTask() > > > amount > > > > > to "post a task" in spec terms, how is it different? > > > > > > I think it's a close enough approximation. Compared to before it uses a > > > different task runner though (timer vs. "generic".) > > > > > > > > Getting microtask checkpoint timing is important and the cause of > failing > > > tests > > > > > in web-platform-tests, so I'm skeptical of changing the timing in any > > > observable > > > > > way other than aligning with that. > > > > > > > > > > Per spec, is it correct that there should be a microtask checkpoint at > > > > > Document::finishedParsing()? > > > > > > IIUC, the reason for that checkpoint is because DOMContentLoaded isn't > posted as > > > a task, so this checkpoint serves to emulate that (i.e step 6 for that > would-be > > > task.) > > > > > > > You may want to consider using the task runner that corresponds to the one > in > > > the spec through here: > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskR... > > > > > > Since the mappings haven't diverged much (yet), that might work. Would need > to > > > look at other cases too though to avoid affecting the ordering. > > > > Probably a TODO for now to revisit it once the mappings are more clear in > TaskRunnerHelper::get()? > > It's not so much about the mapping itself, but rather the actual ordering of the > tasks. From that PoV it might be better to go through the code and note which > task sources should be used for each postTask (and "non-timer Timer"). > > > And regarding the test, do we need to write a separate test for this? I think > WPT has good number of tests for this > /html/semantics/embedded-content/media-elements/loading-the-media-resource/resource-selection*, > so @fs WDYT? > > If they cover microtask semantics well enough then I'm fine with that. Those tests are pretty pedantic (written mostly by Simon Pieters) but why don't we see any of them going from fail to pass in this CL?
skyostil@chromium.org changed reviewers: + haraken@chromium.org
(+haraken@ FYI) > > Probably a TODO for now to revisit it once the mappings are more clear in > TaskRunnerHelper::get()? > > It's not so much about the mapping itself, but rather the actual ordering of the > tasks. From that PoV it might be better to go through the code and note which > task sources should be used for each postTask (and "non-timer Timer"). Could you clarify the ordering issue? The task runners TaskRunnerHelper returns only guarantee ordering between tasks that go into the same task queue. This is the same guarantee that the spec gives. Would we need a stricter guarantee here?
On 2016/09/02 09:55:46, Sami wrote: > (+haraken@ FYI) > > > > Probably a TODO for now to revisit it once the mappings are more clear in > > TaskRunnerHelper::get()? > > > > It's not so much about the mapping itself, but rather the actual ordering of > the > > tasks. From that PoV it might be better to go through the code and note which > > task sources should be used for each postTask (and "non-timer Timer"). > > Could you clarify the ordering issue? The task runners TaskRunnerHelper returns > only guarantee ordering between tasks that go into the same task queue. This is > the same guarantee that the spec gives. Would we need a stricter guarantee here? The actual mapping in TaskRunnerHelper::get() is implementation details and may change in the future. The guarantee the TaskRunnerHelper gives is that tasks that go into the same TaskType (not the same task queue) are guaranteed to be run in order. Does it make sense?
On 2016/09/02 at 09:55:46, skyostil wrote: > (+haraken@ FYI) > > > > Probably a TODO for now to revisit it once the mappings are more clear in > > TaskRunnerHelper::get()? > > > > It's not so much about the mapping itself, but rather the actual ordering of the > > tasks. From that PoV it might be better to go through the code and note which > > task sources should be used for each postTask (and "non-timer Timer"). > > Could you clarify the ordering issue? The task runners TaskRunnerHelper returns only guarantee ordering between tasks that go into the same task queue. This is the same guarantee that the spec gives. Would we need a stricter guarantee here? No, no stricter guarantees, that's exactly the ordering I'm talking about (different task-runners/task-queues for things that per-spec should be from the same task-source.) As such, the mapping doesn't matter, but consistent use of task source/runner/queue does.
https://codereview.chromium.org/1810513002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1810513002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:855: Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, m_resourceSelectionAlgorithmContinuationTask->cancelAndCreate()); Just to be clear, this and the below use of Platform::current()->currentThread()->getWebTaskRunner()->postTask are both places where I think we should either switch to the correct microtask timing, and that using task timing in the interim creates the risk of getting stuck with that timing for a long time instead.
On 2016/09/02 10:31:00, foolip wrote: > https://codereview.chromium.org/1810513002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1810513002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:855: > Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, > m_resourceSelectionAlgorithmContinuationTask->cancelAndCreate()); > Just to be clear, this and the below use of > Platform::current()->currentThread()->getWebTaskRunner()->postTask are both > places where I think we should either switch to the correct microtask timing, > and that using task timing in the interim creates the risk of getting stuck with > that timing for a long time instead. Can we use Microtask for tasks created by CancellableTaskFactory? In our previous discussions we have decided to use CancellableTaskFactory to address the "abort" step of the algorithm. But with the recent changes in Microtask i am not able to use cancellableTaskFactory with Microtask. And as you pointed out the wpt tests behaviour is still not changed with Platform::current()....->postTask changes. I have tried with Microtask::enqueueMicrotask(WTF::bind(&HTMLMediaElement::continueResourceSelectionAlgorithm, wrapPersistent(this))); directly and those tests seem to be passing. But we need to address the "abort" step.
On 2016/09/02 12:55:07, Srirama wrote: > On 2016/09/02 10:31:00, foolip wrote: > > > https://codereview.chromium.org/1810513002/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1810513002/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:855: > > > Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, > > m_resourceSelectionAlgorithmContinuationTask->cancelAndCreate()); > > Just to be clear, this and the below use of > > Platform::current()->currentThread()->getWebTaskRunner()->postTask are both > > places where I think we should either switch to the correct microtask timing, > > and that using task timing in the interim creates the risk of getting stuck > with > > that timing for a long time instead. > > Can we use Microtask for tasks created by CancellableTaskFactory? In our > previous discussions we have decided to use CancellableTaskFactory to address > the "abort" step of the algorithm. But with the recent changes in Microtask i am > not able to use cancellableTaskFactory with Microtask. And as you pointed out > the wpt tests behaviour is still not changed with > Platform::current()....->postTask changes. > I have tried with > Microtask::enqueueMicrotask(WTF::bind(&HTMLMediaElement::continueResourceSelectionAlgorithm, > wrapPersistent(this))); directly and those tests seem to be passing. But we need > to address the "abort" step. Yeah, that is the tricky bit. If there really is no way (Sami probably knows) then an ad-hoc cancellation mechansim that sets a bit to ignore the callback would work.
On 2016/09/02 14:12:11, foolip wrote: > Yeah, that is the tricky bit. If there really is no way (Sami probably knows) > then an ad-hoc cancellation mechansim that sets a bit to ignore the callback > would work. We are working toward supporting WeakPtr-based cancellation in the scheduler (and getting rid of CancellableTaskFactory etc. in the process), so I think enqueuing a Microtask with a WeakPtr for cancellation is the most future proof thing to do here.
On 2016/09/02 14:19:13, Sami wrote: > On 2016/09/02 14:12:11, foolip wrote: > > Yeah, that is the tricky bit. If there really is no way (Sami probably knows) > > then an ad-hoc cancellation mechansim that sets a bit to ignore the callback > > would work. > > We are working toward supporting WeakPtr-based cancellation in the scheduler > (and getting rid of CancellableTaskFactory etc. in the process), so I think > enqueuing a Microtask with a WeakPtr for cancellation is the most future proof > thing to do here. So can i do something similar to ImageLoader::Task in ImageLoader.cpp ?
On 2016/09/06 11:43:30, Srirama wrote: > So can i do something similar to ImageLoader::Task in ImageLoader.cpp ? I guess you could make a Task that contains a WeakPtr, but I suspect just using a WeakPtr in a closure would be less typing.
On 2016/09/06 12:12:14, Sami wrote: > On 2016/09/06 11:43:30, Srirama wrote: > > So can i do something similar to ImageLoader::Task in ImageLoader.cpp ? > > I guess you could make a Task that contains a WeakPtr, but I suspect just using > a WeakPtr in a closure would be less typing. Tried few things for the 2nd option but getting compilation issues, enqueueMicrotask is expecting a std::unique_ptr, so is it possible to get a weakPtr for the same? Couldn't find any example in the code. Can you please give a sample code if you don't mind.
On 2016/09/14 at 13:04:08, srirama.m wrote: > On 2016/09/06 12:12:14, Sami wrote: > > On 2016/09/06 11:43:30, Srirama wrote: > > > So can i do something similar to ImageLoader::Task in ImageLoader.cpp ? > > > > I guess you could make a Task that contains a WeakPtr, but I suspect just using > > a WeakPtr in a closure would be less typing. > > Tried few things for the 2nd option but getting compilation issues, enqueueMicrotask is expecting a std::unique_ptr, so is it possible to get a weakPtr for the same? Couldn't find any example in the code. Can you please give a sample code if you don't mind. Probably best if Sami expands on what he had in mind, but I suspect it might be something along the lines of: WeakPtrFactory<HTMLMediaElement> m_weakTask...; ... enqueueMicrotask(WTF::bind(&HTMLMediaElement::foo, m_weakTask...->createWeakPtr())); (and then invalidate/cancel via revokeAll on the factory. IIUC, this approximately what CancellableTaskFactory does, modulo indirection.) Is that what you tried?
On 2016/09/14 13:24:32, fs wrote: > On 2016/09/14 at 13:04:08, srirama.m wrote: > > On 2016/09/06 12:12:14, Sami wrote: > > > On 2016/09/06 11:43:30, Srirama wrote: > > > > So can i do something similar to ImageLoader::Task in ImageLoader.cpp ? > > > > > > I guess you could make a Task that contains a WeakPtr, but I suspect just > using > > > a WeakPtr in a closure would be less typing. > > > > Tried few things for the 2nd option but getting compilation issues, > enqueueMicrotask is expecting a std::unique_ptr, so is it possible to get a > weakPtr for the same? Couldn't find any example in the code. Can you please give > a sample code if you don't mind. > > Probably best if Sami expands on what he had in mind, but I suspect it might be > something along the lines of: > > WeakPtrFactory<HTMLMediaElement> m_weakTask...; > ... > enqueueMicrotask(WTF::bind(&HTMLMediaElement::foo, > m_weakTask...->createWeakPtr())); > > (and then invalidate/cancel via revokeAll on the factory. IIUC, this > approximately what CancellableTaskFactory does, modulo indirection.) > Thanks fs@, i think this is what Sami also intends and hopefully it should work. > Is that what you tried? I was trying to get a weakPtr directly for the closure without using a weakPtrFactory as i thought that will be of approach1 type. So it failed and anyway approach1 is much more coding than just using weakPtrFactory.
> > Probably best if Sami expands on what he had in mind, but I suspect it might > be > > something along the lines of: > > > > WeakPtrFactory<HTMLMediaElement> m_weakTask...; > > ... > > enqueueMicrotask(WTF::bind(&HTMLMediaElement::foo, > > m_weakTask...->createWeakPtr())); > > > > (and then invalidate/cancel via revokeAll on the factory. IIUC, this > > approximately what CancellableTaskFactory does, modulo indirection.) > > > Thanks fs@, i think this is what Sami also intends and hopefully it should work. Yep, that's exactly right -- thanks Fredrik!
Patchset #7 (id:200001) has been deleted
PTAL. 1. Updated two test cases to match the new behavior. 2. Still the old issue (microtask checkpoint in document.cpp) is there and two test cases are failing. There isn't much progress in https://crbug.com/425790. Any suggestion how to proceed? whether to wait for 425790 to be fixed or try something different?
On 2016/09/17 13:34:27, Srirama wrote: > PTAL. > 1. Updated two test cases to match the new behavior. > 2. Still the old issue (microtask checkpoint in document.cpp) is there and two > test cases are failing. > There isn't much progress in https://crbug.com/425790. > Any suggestion how to proceed? whether to wait for 425790 to be fixed or try > something different? I asked kouhei@ for an update on the issue.
On 2016/03/17 12:05:08, alex clarke wrote: > +skyostil@ > > On 2016/03/17 11:30:55, philipj_UTC7 wrote: > > sof@, alexclarke@, you're the top pokers of CancellableTaskFactoryTest.cpp, so > I > > hope you can advise on this situation. > > > > In the HTML spec, media elements have their own task source, which is used for > > queueing tasks for fire events and many other things. At any point, all > pending > > tasks can be canceled, this happens when starting a new resource loads so that > > tasks/events from a previous resource are not misattributed (by event > handlers) > > as being about the new source. > > > > The spec also says "await a stable state" in a few algorithms that can can be > > aborted with language like "Abort any already-running instance of the resource > > selection algorithm for this element." This amounts to cancelable microtasks. > > > > What do you make of this situation? I see a few options: > > > > 1. A media element-specific type of callback object, that can be used for both > > tasks and microtasks, where HTMLMediaElement keeps track of all pending tasks > > and has a state bit for each to neuter them, so that their callback becomes a > > no-op. > > > > 2. Adapting CancellableTaskFactory to be able to create any number of pending > > tasks that can all be canceled as a group. AFAICT the shared type for > microtasks > > and tasks is SameThreadClosure, but CancellableTaskFactory makes > > WebTaskRunner::Task. Not sure if this is trivial or hard to bridge. > The CancellableTaskFactory actually builds a SameThreadClosure, so getting it to > instead accept one should be easy. > > Mass cancellation is possibly a little trickier. Would individual cancellation > still be needed? If not then a group could all share the same weak pointer. > We'd probably need a new class based on CancellableTaskFactory. Not sure if this is still relevant, but FWIW the scheduler now has a fast path for detecting cancelled tasks (via weak pointers). dchang@ has been advocating a pattern like so: WeakPtrFactory<SomeClass> m_factory; web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func1, m_factory.createWeakPtr())); web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func2, m_factory.createWeakPtr())); web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func3, m_factory.createWeakPtr())); web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func4, m_factory.createWeakPtr())); ... m_factory.revokeAll(); // Cancels all pending tasks. > > > > > 3. Fiddle with task and microtask queues to make it possible to actually > remove > > tasks from them. Presumably this is very hard/messy, but it's worth asking. > This is really tricky under the hood since both the chromium base message loop > and the render scheduler have been built with the assumption that cancellation > is the responsibility of the task itself. Data structures like std::queue and > std::priority_queue have been used that don't support arbitrary deletion. > > > > > WDYT? > > I can think of a few other ways this could be done: > > 4. We could add a new API that lets blink create a WebTaskRunner for media > elements, along with something to allow mass-cancellation of all tasks posted on > it. This has some nice side effects for frame blamer and tracing. > > 5. I'm not sure if it's applicable here, but I have a C++ promises demo that has > a dependency graph of tasks (i.e. Task D runs after A, B & C and if you cancel > task D then A, B & C no longer need to run). > https://codereview.chromium.org/1401553002/ > > > > > > > https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: > > Microtask::enqueueMicrotask(task.release()); > > On 2016/03/16 17:41:17, Srirama wrote: > > > Infact when i started i just copied it from ImageLoader but i realized that > we > > > need it for aborting the already running instance. This is the only reason. > > > > OK. So this problem has come up several times recently. To make > HTMLMediaElement > > more per spec, we really need both tasks and microtasks that can be canceled. > > Events should also also be fired using such tasks, replacing the > > m_asyncEventQueue. > > > > Right now the best candidate for all of this is CancellableTaskFactory, but > IIRC > > it can only have one task in flight at a time, because that's just what > > cancelAndCreate does. I'll add a few reviewers and ask for advice.
On 2016/09/22 10:18:16, alex clarke wrote: > On 2016/03/17 12:05:08, alex clarke wrote: > > +skyostil@ > > > > On 2016/03/17 11:30:55, philipj_UTC7 wrote: > > > sof@, alexclarke@, you're the top pokers of CancellableTaskFactoryTest.cpp, > so > > I > > > hope you can advise on this situation. > > > > > > In the HTML spec, media elements have their own task source, which is used > for > > > queueing tasks for fire events and many other things. At any point, all > > pending > > > tasks can be canceled, this happens when starting a new resource loads so > that > > > tasks/events from a previous resource are not misattributed (by event > > handlers) > > > as being about the new source. > > > > > > The spec also says "await a stable state" in a few algorithms that can can > be > > > aborted with language like "Abort any already-running instance of the > resource > > > selection algorithm for this element." This amounts to cancelable > microtasks. > > > > > > What do you make of this situation? I see a few options: > > > > > > 1. A media element-specific type of callback object, that can be used for > both > > > tasks and microtasks, where HTMLMediaElement keeps track of all pending > tasks > > > and has a state bit for each to neuter them, so that their callback becomes > a > > > no-op. > > > > > > 2. Adapting CancellableTaskFactory to be able to create any number of > pending > > > tasks that can all be canceled as a group. AFAICT the shared type for > > microtasks > > > and tasks is SameThreadClosure, but CancellableTaskFactory makes > > > WebTaskRunner::Task. Not sure if this is trivial or hard to bridge. > > The CancellableTaskFactory actually builds a SameThreadClosure, so getting it > to > > instead accept one should be easy. > > > > Mass cancellation is possibly a little trickier. Would individual > cancellation > > still be needed? If not then a group could all share the same weak pointer. > > We'd probably need a new class based on CancellableTaskFactory. > > Not sure if this is still relevant, but FWIW the scheduler now has a fast path > for detecting cancelled tasks > (via weak pointers). > > dchang@ has been advocating a pattern like so: > > WeakPtrFactory<SomeClass> m_factory; > > web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func1, > m_factory.createWeakPtr())); > web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func2, > m_factory.createWeakPtr())); > web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func3, > m_factory.createWeakPtr())); > web_task_runner->postTask(BLINK_FROM_HERE, WTF::bind(&SomeClass::func4, > m_factory.createWeakPtr())); > > ... > > m_factory.revokeAll(); // Cancels all pending tasks. > Thanks Alex, my latest patcheset already has the changes based on this pattern, though we are using Microtask instead of webtaskrunner. > > > > > > > > 3. Fiddle with task and microtask queues to make it possible to actually > > remove > > > tasks from them. Presumably this is very hard/messy, but it's worth asking. > > This is really tricky under the hood since both the chromium base message loop > > and the render scheduler have been built with the assumption that cancellation > > is the responsibility of the task itself. Data structures like std::queue and > > std::priority_queue have been used that don't support arbitrary deletion. > > > > > > > > WDYT? > > > > I can think of a few other ways this could be done: > > > > 4. We could add a new API that lets blink create a WebTaskRunner for media > > elements, along with something to allow mass-cancellation of all tasks posted > on > > it. This has some nice side effects for frame blamer and tracing. > > > > 5. I'm not sure if it's applicable here, but I have a C++ promises demo that > has > > a dependency graph of tasks (i.e. Task D runs after A, B & C and if you cancel > > task D then A, B & C no longer need to run). > > https://codereview.chromium.org/1401553002/ > > > > > > > > > > > > > https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > > > > > https://codereview.chromium.org/1810513002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:896: > > > Microtask::enqueueMicrotask(task.release()); > > > On 2016/03/16 17:41:17, Srirama wrote: > > > > Infact when i started i just copied it from ImageLoader but i realized > that > > we > > > > need it for aborting the already running instance. This is the only > reason. > > > > > > OK. So this problem has come up several times recently. To make > > HTMLMediaElement > > > more per spec, we really need both tasks and microtasks that can be > canceled. > > > Events should also also be fired using such tasks, replacing the > > > m_asyncEventQueue. > > > > > > Right now the best candidate for all of this is CancellableTaskFactory, but > > IIRC > > > it can only have one task in flight at a time, because that's just what > > > cancelAndCreate does. I'll add a few reviewers and ask for advice. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
