Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647453003:
Use timer task queue for RenderMediaLog
|
Created:
3 years, 11 months ago by altimin Modified:
3 years, 10 months ago CC:
chfremer+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse timer task queue for RenderMediaLog
As a part of effort to post tasks to correct scheduler task queues instead
of default one, use frame-specific timer task runner for RenderMediaLog tasks.
BUG=676805
Review-Url: https://codereview.chromium.org/2647453003
Cr-Commit-Position: refs/heads/master@{#448777}
Committed: https://chromium.googlesource.com/chromium/src/+/c4d24a8a8d97013513f2f668734ce608a82a541a
Patch Set 1 #Patch Set 2 : Rebase #
Messages
Total messages: 36 (18 generated)
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...
altimin@chromium.org changed reviewers: + dalecurtis@chromium.org, jochen@chromium.org
+dalecurtis@ for content/renderer/media +jochen@ for content/ PTAL
Can you elaborate on any consequence of moving these off the render thread? This is used to deliver media events to our chrome://media-internals page. This means the page generating these is frequently in the background. I don't think they need to be real time or anything, but they do need some frequency of update.
On 2017/01/18 18:19:50, DaleCurtis wrote: > Can you elaborate on any consequence of moving these off the render thread? This > is used to deliver media events to our chrome://media-internals page. This means > the page generating these is frequently in the background. I don't think they > need to be real time or anything, but they do need some frequency of update. The thread stays the same, but attribution of tasks in renderer scheduler changes from default tq to frame-specifc timer tq. Also it becomes possible to throttle these events in background pages to 1s.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
1s should be fine, we already rate limit to that in our own code, so it might be worth deduping. https://cs.chromium.org/chromium/src/content/renderer/media/render_media_log.... lgtm
is it possible to add a test for this?
On 2017/01/19 19:01:01, jochen wrote: > is it possible to add a test for this? browser tests should cover these type of changes.
On 2017/01/20 at 14:03:43, altimin wrote: > On 2017/01/19 19:01:01, jochen wrote: > > is it possible to add a test for this? > > browser tests should cover these type of changes. can you add one then? The CL description says it's now the correct task queue, so I assume before it wasn't the correct one, and it should be possible to test the correctness, no?
On 2017/01/23 08:29:30, jochen wrote: > On 2017/01/20 at 14:03:43, altimin wrote: > > On 2017/01/19 19:01:01, jochen wrote: > > > is it possible to add a test for this? > > > > browser tests should cover these type of changes. > > can you add one then? The CL description says it's now the correct task queue, > so I assume before it wasn't the correct one, and it should be possible to test > the correctness, no? I meant that existing layout tests should provide good coverage for this CL. Maybe "correct task queue" isn't the best wording. Currently it's the default queue, which is OK from correctness standpoint, but does not give the scheduler flexibility to prioritise between different types of tasks, frames, etc.
sorry for insisting here, but either the current version is OK, or there should be a way to verify that the new version does something the old one didn't
after chatting a bit offline, this change lgtm, but we should have a discussion about the long term testing plan
On 2017/01/27 04:10:08, jochen (travelling til Feb 4) wrote: > after chatting a bit offline, this change lgtm, but we should have a discussion > about the long term testing plan I recently drafted a plan to make tasks to appropriate task queues: https://docs.google.com/document/d/1PVXruW1rGfobdHlFNX70izWbwT5BfWCCk2CEsCSeYjQ/ Should discuss it offline on Blinkon?
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/01/27 at 10:26:50, altimin wrote: > On 2017/01/27 04:10:08, jochen (travelling til Feb 4) wrote: > > after chatting a bit offline, this change lgtm, but we should have a discussion > > about the long term testing plan > > I recently drafted a plan to make tasks to appropriate task queues: https://docs.google.com/document/d/1PVXruW1rGfobdHlFNX70izWbwT5BfWCCk2CEsCSeYjQ/ > Should discuss it offline on Blinkon? sgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2647453003/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486506432857590,
"parent_rev": "5050e17b6d5259f873447ffc57bf274af2a189a1", "commit_rev":
"c4d24a8a8d97013513f2f668734ce608a82a541a"}
Message was sent while issue was closed.
Description was changed from ========== Use timer task queue for RenderMediaLog As a part of effort to post tasks to correct scheduler task queues instead of default one, use frame-specific timer task runner for RenderMediaLog tasks. BUG=676805 ========== to ========== Use timer task queue for RenderMediaLog As a part of effort to post tasks to correct scheduler task queues instead of default one, use frame-specific timer task runner for RenderMediaLog tasks. BUG=676805 Review-Url: https://codereview.chromium.org/2647453003 Cr-Commit-Position: refs/heads/master@{#448777} Committed: https://chromium.googlesource.com/chromium/src/+/c4d24a8a8d97013513f2f668734c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c4d24a8a8d97013513f2f668734c...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2688523002/ by altimin@chromium.org. The reason for reverting is: VideoFullscreenOrientationLockTests became flaky (see crbug.com/676070).
Message was sent while issue was closed.
Description was changed from ========== Use timer task queue for RenderMediaLog As a part of effort to post tasks to correct scheduler task queues instead of default one, use frame-specific timer task runner for RenderMediaLog tasks. BUG=676805 Review-Url: https://codereview.chromium.org/2647453003 Cr-Commit-Position: refs/heads/master@{#448777} Committed: https://chromium.googlesource.com/chromium/src/+/c4d24a8a8d97013513f2f668734c... ========== to ========== Use timer task queue for RenderMediaLog As a part of effort to post tasks to correct scheduler task queues instead of default one, use frame-specific timer task runner for RenderMediaLog tasks. BUG=676805 Review-Url: https://codereview.chromium.org/2647453003 Cr-Commit-Position: refs/heads/master@{#448777} Committed: https://chromium.googlesource.com/chromium/src/+/c4d24a8a8d97013513f2f668734c... ==========
The CQ bit was checked by altimin@chromium.org
The CQ bit was unchecked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
