Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713613002:
Convert HTMLMediaElement timers to MediaElementEvent per-frame. (Closed)
|
Created:
3 years, 10 months ago by joelhockey Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert HTMLMediaElement timers to MediaElementEvent per-frame.
Include m_deferredLoadTimer in didMoveToNewDocument.
My reading of the spec suggests that all of these timers should use MediaElementEvent task source. I am quite sure about the load and progress timers. The viewport-related timers may be different, but I see no reason not to have them use the same task source as the other timers.
https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements
"Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question."
BUG=624694
Patch Set 1 #
Total comments: 2
Patch Set 2 : merge #Messages
Total messages: 15 (8 generated)
Description was changed from ========== Convert timers to MediaElementEvent per-frame. BUG=624694 ========== to ========== Convert HTMLMediaElement timers to MediaElementEvent per-frame. Update references to spec (mostly 4.8.10 -> 4.8.12) Include m_deferredLoadTimer in didMoveToNewDocument. My reading of the spec suggests that all of these timers should use MediaElementEvent task source. I am quite sure about the load and progress timers. The viewport-related timers may be different, but I see no reason not to have them use the same task source as the other timers. https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." altimin, dcheng, mlamouri, I see you were involved in the previous CL to modify these timers, I want to check if you have any thoughts. BUG=624694 ==========
Description was changed from ========== Convert HTMLMediaElement timers to MediaElementEvent per-frame. Update references to spec (mostly 4.8.10 -> 4.8.12) Include m_deferredLoadTimer in didMoveToNewDocument. My reading of the spec suggests that all of these timers should use MediaElementEvent task source. I am quite sure about the load and progress timers. The viewport-related timers may be different, but I see no reason not to have them use the same task source as the other timers. https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." altimin, dcheng, mlamouri, I see you were involved in the previous CL to modify these timers, I want to check if you have any thoughts. BUG=624694 ========== to ========== Convert HTMLMediaElement timers to MediaElementEvent per-frame. Update references to spec (mostly 4.8.10 -> 4.8.12) Include m_deferredLoadTimer in didMoveToNewDocument. My reading of the spec suggests that all of these timers should use MediaElementEvent task source. I am quite sure about the load and progress timers. The viewport-related timers may be different, but I see no reason not to have them use the same task source as the other timers. https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." BUG=624694 ==========
haraken, PTAL. altimin, dcheng, mlamouri, I see you were involved in the previous CL to modify these timers, I want to check if you have any thoughts.
First of all, this CL is making multiple non-trivial changes in one go. Let's split the CL into pieces. > "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." This does not mean that all tasks around HTMLMediaElement should use the media element event task source. It is just saying that all the tasks defined in the spec should use the media element event task source. Blink's internal tasks should not use the media element event task source. I think that those tasks are Blink's internal tasks (i.e., unspeced tasks). Also they should not be throttled for UX reasons, so it would make sense to use the Unthrottled task runner. altimin@: BTW, if there is a speced task that should not be throttled for UX reasons, what task runner should we use? If we use the Unthrottled task runner, it will break the ordering requirement of the task. If we use the speced task runner (e.g., networking task source), it might be throttled.
Description was changed from ========== Convert HTMLMediaElement timers to MediaElementEvent per-frame. Update references to spec (mostly 4.8.10 -> 4.8.12) Include m_deferredLoadTimer in didMoveToNewDocument. My reading of the spec suggests that all of these timers should use MediaElementEvent task source. I am quite sure about the load and progress timers. The viewport-related timers may be different, but I see no reason not to have them use the same task source as the other timers. https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." BUG=624694 ========== to ========== Convert HTMLMediaElement timers to MediaElementEvent per-frame. Include m_deferredLoadTimer in didMoveToNewDocument. My reading of the spec suggests that all of these timers should use MediaElementEvent task source. I am quite sure about the load and progress timers. The viewport-related timers may be different, but I see no reason not to have them use the same task source as the other timers. https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." BUG=624694 ==========
On 2017/02/22 at 10:27:42, haraken wrote: > First of all, this CL is making multiple non-trivial changes in one go. Let's split the CL into pieces. > > > "Except where otherwise explicitly specified, the task source for all the tasks queued in this section and its subsections is the media element event task source of the media element in question." > > This does not mean that all tasks around HTMLMediaElement should use the media element event task source. It is just saying that all the tasks defined in the spec should use the media element event task source. Blink's internal tasks should not use the media element event task source. > > I think that those tasks are Blink's internal tasks (i.e., unspeced tasks). Also they should not be throttled for UX reasons, so it would make sense to use the Unthrottled task runner. > > altimin@: BTW, if there is a speced task that should not be throttled for UX reasons, what task runner should we use? If we use the Unthrottled task runner, it will break the ordering requirement of the task. If we use the speced task runner (e.g., networking task source), it might be throttled. Moved spec section comment fixes into https://codereview.chromium.org/2706333003
The CQ bit was checked by altimin@chromium.org 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...
https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:524: TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())); This is a change in behaviour, right? https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.h:618: TaskRunnerTimer<HTMLMediaElement> m_deferredLoadTimer; Why wasn't it a TaskRunnerTimer<> like the others?
On 2017/02/22 10:27:42, haraken wrote: > First of all, this CL is making multiple non-trivial changes in one go. Let's > split the CL into pieces. > > > "Except where otherwise explicitly specified, the task source for all the > tasks queued in this section and its subsections is the media element event task > source of the media element in question." > > This does not mean that all tasks around HTMLMediaElement should use the media > element event task source. It is just saying that all the tasks defined in the > spec should use the media element event task source. Blink's internal tasks > should not use the media element event task source. > > I think that those tasks are Blink's internal tasks (i.e., unspeced tasks). Also > they should not be throttled for UX reasons, so it would make sense to use the > Unthrottled task runner. +1. However, Daniel knows the answer, let's wait for him. Also, I think each time we use unthrottled task runner explicitly, we need to write a comment why we're doing this. > altimin@: BTW, if there is a speced task that should not be throttled for UX > reasons, what task runner should we use? If we use the Unthrottled task runner, > it will break the ordering requirement of the task. If we use the speced task > runner (e.g., networking task source), it might be throttled. I think that we need to come up with a stop of throttling which preserves UX. Satisfying ordering requirements should be a priority for us (web compatibility!). Let's discuss it in further detail when we have an example.
On 2017/02/22 12:10:04, mlamouri wrote: > https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:524: > TaskRunnerHelper::get(TaskType::MediaElementEvent, &document())); > This is a change in behaviour, right? Well, yes and no. The problem is that we currently do not throttle these events: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TaskR... But we will at some point. > https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): > > https://codereview.chromium.org/2713613002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.h:618: > TaskRunnerTimer<HTMLMediaElement> m_deferredLoadTimer; > Why wasn't it a TaskRunnerTimer<> like the others?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
After splitting the CL out, it now only has changes to convert all timers from Unthrottled to MediaElementEvent. After reading the spec, I did originally think that MediaElementEvent would be the correct task source, but I think I might just close this CL since there doesn't seem to be easy agreement about what task type to use. |
