Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1939943002:
Increase cases where back/front buffering early rendering occur. (Closed)
|
Created:
4 years, 7 months ago by DaleCurtis Modified:
4 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fast_release Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease cases where back/front buffering early rendering occur.
When dropped frames occur that are opporunties to render to the
front buffer when more than one frame is available. Likewise if
a later frame is chosen for front buffer rendering, frames earlier
than it can be ignored when considering if the back buffer is open.
There are also cases where the renderer has dropped the most recent
frame and is preparing to display the frame, this releases slightly
ahead of schedule assuming normal operation. In less than ideal
circumstances the wrong frame may be released.
BUG=601066
TEST=none
Committed: https://crrev.com/8a12536eefe4a11a23d33676fb73c71885159e02
Cr-Commit-Position: refs/heads/master@{#391876}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix front index calculation. #
Depends on Patchset: Messages
Total messages: 20 (6 generated)
dalecurtis@chromium.org changed reviewers: + liberato@chromium.org, watk@chromium.org
lgtm % suggestion for possible additional concurrency. https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... media/gpu/android_deferred_rendering_backing_strategy.cc:299: first_renderable_image->UpdateSurface( this will force a wait for frame available after your other CL. unsure if that's a win or not, especially when we're on a device that often hits the OnFrameAvailable timeout. the alternative of waiting during drawing seems worse, but we might not draw this frame at all and we're holding up frame delivery and (potentially) the next draw here anyway. the alternative is to do this only when the signal has arrived or the timeout has elapsed. in that case, consider having OnFrameAvailable post a task back to the main thread to MaybeRenderEarly.
https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... media/gpu/android_deferred_rendering_backing_strategy.cc:299: first_renderable_image->UpdateSurface( On 2016/05/03 at 15:20:55, liberato wrote: > this will force a wait for frame available after your other CL. unsure if that's a win or not, especially when we're on a device that often hits the OnFrameAvailable timeout. the alternative of waiting during drawing seems worse, but we might not draw this frame at all and we're holding up frame delivery and (potentially) the next draw here anyway. > > the alternative is to do this only when the signal has arrived or the timeout has elapsed. in that case, consider having OnFrameAvailable post a task back to the main thread to MaybeRenderEarly. I don't follow. The render to front buffer above always waits since we release to the front buffer, this on the other hand should release but not wait.
https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... media/gpu/android_deferred_rendering_backing_strategy.cc:299: first_renderable_image->UpdateSurface( On 2016/05/03 21:13:34, DaleCurtis wrote: > On 2016/05/03 at 15:20:55, liberato wrote: > > this will force a wait for frame available after your other CL. unsure if > that's a win or not, especially when we're on a device that often hits the > OnFrameAvailable timeout. the alternative of waiting during drawing seems > worse, but we might not draw this frame at all and we're holding up frame > delivery and (potentially) the next draw here anyway. > > > > the alternative is to do this only when the signal has arrived or the timeout > has elapsed. in that case, consider having OnFrameAvailable post a task back to > the main thread to MaybeRenderEarly. > > I don't follow. The render to front buffer above always waits since we release > to the front buffer, this on the other hand should release but not wait. hrm, my comment was pretty jumbly. sorry about that. my concern is that we'll now incur a WaitForFrameAvailable for frames that we don't ever draw, and we're still waiting on the critical path. right now, this tries "render to front buffer" then "render to back buffer". however, it might do better to "render to back buffer" and "promote back buffer to front buffer". then it's easy to defer the promotion until after the frame available has happened. after your other CL (https://codereview.chromium.org/1924973004/), we'll have that ability pretty easily. might need to wait for your other CL to land, though.
Patchset #2 (id:20001) has been deleted
PTAL, the front index calculation was also incorrect. Fixed. https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... media/gpu/android_deferred_rendering_backing_strategy.cc:299: first_renderable_image->UpdateSurface( On 2016/05/03 at 21:31:49, liberato wrote: > On 2016/05/03 21:13:34, DaleCurtis wrote: > > On 2016/05/03 at 15:20:55, liberato wrote: > > > this will force a wait for frame available after your other CL. unsure if > > that's a win or not, especially when we're on a device that often hits the > > OnFrameAvailable timeout. the alternative of waiting during drawing seems > > worse, but we might not draw this frame at all and we're holding up frame > > delivery and (potentially) the next draw here anyway. > > > > > > the alternative is to do this only when the signal has arrived or the timeout > > has elapsed. in that case, consider having OnFrameAvailable post a task back to > > the main thread to MaybeRenderEarly. > > > > I don't follow. The render to front buffer above always waits since we release > > to the front buffer, this on the other hand should release but not wait. > > hrm, my comment was pretty jumbly. sorry about that. > > my concern is that we'll now incur a WaitForFrameAvailable for frames that we don't ever draw, and we're still waiting on the critical path. > > right now, this tries "render to front buffer" then "render to back buffer". however, it might do better to "render to back buffer" and "promote back buffer to front buffer". then it's easy to defer the promotion until after the frame available has happened. > > after your other CL (https://codereview.chromium.org/1924973004/), we'll have that ability pretty easily. > > might need to wait for your other CL to land, though. So you're advocating that in the case where we have neither a front buffer nor a back buffer, that we only render the back buffer and let the promotion happen the next time this method is called or during the draw phase. This seemed reasonable, so I tried it, but it has the opposite of the intended effect, everything slows down since the MediaCodec is hitting the stalled state for lack of buffers again.
On 2016/05/04 00:01:33, DaleCurtis wrote: > PTAL, the front index calculation was also incorrect. Fixed. > > https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... > File media/gpu/android_deferred_rendering_backing_strategy.cc (right): > > https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... > media/gpu/android_deferred_rendering_backing_strategy.cc:299: > first_renderable_image->UpdateSurface( > On 2016/05/03 at 21:31:49, liberato wrote: > > On 2016/05/03 21:13:34, DaleCurtis wrote: > > > On 2016/05/03 at 15:20:55, liberato wrote: > > > > this will force a wait for frame available after your other CL. unsure if > > > that's a win or not, especially when we're on a device that often hits the > > > OnFrameAvailable timeout. the alternative of waiting during drawing seems > > > worse, but we might not draw this frame at all and we're holding up frame > > > delivery and (potentially) the next draw here anyway. > > > > > > > > the alternative is to do this only when the signal has arrived or the > timeout > > > has elapsed. in that case, consider having OnFrameAvailable post a task > back to > > > the main thread to MaybeRenderEarly. > > > > > > I don't follow. The render to front buffer above always waits since we > release > > > to the front buffer, this on the other hand should release but not wait. > > > > hrm, my comment was pretty jumbly. sorry about that. > > > > my concern is that we'll now incur a WaitForFrameAvailable for frames that we > don't ever draw, and we're still waiting on the critical path. > > > > right now, this tries "render to front buffer" then "render to back buffer". > however, it might do better to "render to back buffer" and "promote back buffer > to front buffer". then it's easy to defer the promotion until after the frame > available has happened. > > > > after your other CL (https://codereview.chromium.org/1924973004/), we'll have > that ability pretty easily. > > > > might need to wait for your other CL to land, though. > > So you're advocating that in the case where we have neither a front buffer nor a > back buffer, that we only render the back buffer and let the promotion happen > the next time this method is called or during the draw phase. > > This seemed reasonable, so I tried it, but it has the opposite of the intended > effect, everything slows down since the MediaCodec is hitting the stalled state > for lack of buffers again. lgtm.
On 2016/05/04 00:01:33, DaleCurtis wrote: > PTAL, the front index calculation was also incorrect. Fixed. > > https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... > File media/gpu/android_deferred_rendering_backing_strategy.cc (right): > > https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... > media/gpu/android_deferred_rendering_backing_strategy.cc:299: > first_renderable_image->UpdateSurface( > On 2016/05/03 at 21:31:49, liberato wrote: > > On 2016/05/03 21:13:34, DaleCurtis wrote: > > > On 2016/05/03 at 15:20:55, liberato wrote: > > > > this will force a wait for frame available after your other CL. unsure if > > > that's a win or not, especially when we're on a device that often hits the > > > OnFrameAvailable timeout. the alternative of waiting during drawing seems > > > worse, but we might not draw this frame at all and we're holding up frame > > > delivery and (potentially) the next draw here anyway. > > > > > > > > the alternative is to do this only when the signal has arrived or the > timeout > > > has elapsed. in that case, consider having OnFrameAvailable post a task > back to > > > the main thread to MaybeRenderEarly. > > > > > > I don't follow. The render to front buffer above always waits since we > release > > > to the front buffer, this on the other hand should release but not wait. > > > > hrm, my comment was pretty jumbly. sorry about that. > > > > my concern is that we'll now incur a WaitForFrameAvailable for frames that we > don't ever draw, and we're still waiting on the critical path. > > > > right now, this tries "render to front buffer" then "render to back buffer". > however, it might do better to "render to back buffer" and "promote back buffer > to front buffer". then it's easy to defer the promotion until after the frame > available has happened. > > > > after your other CL (https://codereview.chromium.org/1924973004/), we'll have > that ability pretty easily. > > > > might need to wait for your other CL to land, though. > > So you're advocating that in the case where we have neither a front buffer nor a > back buffer, that we only render the back buffer and let the promotion happen > the next time this method is called or during the draw phase. > > This seemed reasonable, so I tried it, but it has the opposite of the intended > effect, everything slows down since the MediaCodec is hitting the stalled state > for lack of buffers again. lgtm.
https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... File media/gpu/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1939943002/diff/1/media/gpu/android_deferred_... media/gpu/android_deferred_rendering_backing_strategy.cc:299: first_renderable_image->UpdateSurface( On 2016/05/04 00:01:33, DaleCurtis wrote: > On 2016/05/03 at 21:31:49, liberato wrote: > > On 2016/05/03 21:13:34, DaleCurtis wrote: > > > On 2016/05/03 at 15:20:55, liberato wrote: > > > > this will force a wait for frame available after your other CL. unsure if > > > that's a win or not, especially when we're on a device that often hits the > > > OnFrameAvailable timeout. the alternative of waiting during drawing seems > > > worse, but we might not draw this frame at all and we're holding up frame > > > delivery and (potentially) the next draw here anyway. > > > > > > > > the alternative is to do this only when the signal has arrived or the > timeout > > > has elapsed. in that case, consider having OnFrameAvailable post a task > back to > > > the main thread to MaybeRenderEarly. > > > > > > I don't follow. The render to front buffer above always waits since we > release > > > to the front buffer, this on the other hand should release but not wait. > > > > hrm, my comment was pretty jumbly. sorry about that. > > > > my concern is that we'll now incur a WaitForFrameAvailable for frames that we > don't ever draw, and we're still waiting on the critical path. > > > > right now, this tries "render to front buffer" then "render to back buffer". > however, it might do better to "render to back buffer" and "promote back buffer > to front buffer". then it's easy to defer the promotion until after the frame > available has happened. > > > > after your other CL (https://codereview.chromium.org/1924973004/), we'll have > that ability pretty easily. > > > > might need to wait for your other CL to land, though. > > So you're advocating that in the case where we have neither a front buffer nor a > back buffer, that we only render the back buffer and let the promotion happen > the next time this method is called or during the draw phase. > > This seemed reasonable, so I tried it, but it has the opposite of the intended > effect, everything slows down since the MediaCodec is hitting the stalled state > for lack of buffers again. hrm, that's unexpected, since we're still releasing the buffer from media codec's point of view. it must be resulting in fewer releases, meaning that we're spending more time waiting to get the cpu back on the gpu main thread (or that 10msec DoIOTask poll is too slow) than the frame available timer takes. oh, well. thanks for trying it out.
lgtm
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1939943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1939943002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1939943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1939943002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Increase cases where back/front buffering early rendering occur. When dropped frames occur that are opporunties to render to the front buffer when more than one frame is available. Likewise if a later frame is chosen for front buffer rendering, frames earlier than it can be ignored when considering if the back buffer is open. There are also cases where the renderer has dropped the most recent frame and is preparing to display the frame, this releases slightly ahead of schedule assuming normal operation. In less than ideal circumstances the wrong frame may be released. BUG=601066 TEST=none ========== to ========== Increase cases where back/front buffering early rendering occur. When dropped frames occur that are opporunties to render to the front buffer when more than one frame is available. Likewise if a later frame is chosen for front buffer rendering, frames earlier than it can be ignored when considering if the back buffer is open. There are also cases where the renderer has dropped the most recent frame and is preparing to display the frame, this releases slightly ahead of schedule assuming normal operation. In less than ideal circumstances the wrong frame may be released. BUG=601066 TEST=none Committed: https://crrev.com/8a12536eefe4a11a23d33676fb73c71885159e02 Cr-Commit-Position: refs/heads/master@{#391876} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8a12536eefe4a11a23d33676fb73c71885159e02 Cr-Commit-Position: refs/heads/master@{#391876} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
