ReleaseOutputBuffer to surface back and front buffers where possible.

ReleaseOutputBuffer to surface back and front buffers where possible.

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

This proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for some more details on the off screen issue.

This patch unifies all calls from the deferred strategy for releasing
output buffers into a single AVDACodecImage::ReleaseOutputBuffer() call
with a couple modes to support all the cases we use:
- skip render: ReleaseOutputBuffer(, false);
- release only: ReleaseOutputBuffer(, true); FrameListener.Wait();
- release+update: same as "release only" but also UpdateTexImage().

BUG=601066
TEST=manual testing on n7v2 and moto x.

Committed: https://crrev.com/1ee5b842e11f679b21a38f156fb7d4688ff8665a
Cr-Commit-Position: refs/heads/master@{#389350}

[DaleCurtis, 2016-04-15 00:48:21]

dalecurtis@chromium.org changed reviewers:
+ liberato@chromium.org, sandersd@chromium.org, watk@chromium.org

[DaleCurtis, 2016-04-15 00:48:21]

If you guys have time Fri/Mon/Tue to pick this up it's all yours. It needs some
work to do the following:
- wait for the OnFrameAvailable listener to fire before queuing the next release
or updateteximage.
- made eminently less hackish.
- disable back buffer usage for SurfaceView, though promotion to front can be
kept when only 1 frame is outstanding.

Can be tested using these test pages:

http://storage.googleapis.com/dalecurtis/tag.html?src=buck2.mp4
http://storage.googleapis.com/dalecurtis/n7slow.mp4 (after setting
adaptivePlayback=false).

[DaleCurtis, 2016-04-21 22:40:14]

Patchset #3 (id:40001) has been deleted

[DaleCurtis, 2016-04-21 22:40:59]

Description was changed from

==========
DO NOT COMMIT - release codec buffer, v2

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

this proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for details on the bg render issue.

BUG=

patch from issue 1864763002 at patchset 20001
(http://crrev.com/1864763002#ps20001)
==========

to

==========
DO NOT COMMIT - release codec buffer, v2

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

this proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for details on the bg render issue.

BUG=603358
==========

[DaleCurtis, 2016-04-21 22:41:17]

Cleaned this up a lot. Please look over it and see if it looks sane. This
refactors the entire releasing mechanism for the codec image.

[DaleCurtis, 2016-04-22 00:39:11]

Description was changed from

==========
DO NOT COMMIT - release codec buffer, v2

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

this proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for details on the bg render issue.

BUG=603358
==========

to

==========
ReleaseOutputBuffer to surface back and front buffers where possible.

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

This proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for some more details on the off screen issue.

This patch unifies all calls from the deferred strategy for releasing
output buffers into a single AVDACodecImage::ReleaseOutputBuffer() call
with a couple modes to support all the cases we use:
- skip render: ReleaseOutputBuffer(, false);
- release only: ReleaseOutputBuffer(, true); FrameListener.Wait();
- release+update: same as "release only" but also UpdateTexImage().

BUG=601066
TEST=manual testing on n7v2 and moto x.
==========

[liberato (no reviews please), 2016-04-22 00:51:06]

i don't have time tonight to look over in more detail, but i think these
suggetions will simplify things a bit.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:512:
strategy_->MaybeRenderEarly(pictures_out_for_display_);
i see why you'd want to call this periodically, but why twice?

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:80: const
std::vector<int32_t>& pictures_out_for_display) = 0;
couldn't pictures_out_for_display be an implementation detail of the deferred
strategy, rather than passing it around?  i think that it gets all the callbacks
that it needs to keep it up to date.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_codec_image.cc (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:114:
ReleaseOutputBuffer(ReleaseMode::RENDER_ONLY);
RENDER_AND_UPDATE, else it will try to wait for frame available.  or, if you
like my new naming scheme, RENDER_TO_FRONT_BUFFER.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:176: if (release_mode ==
ReleaseMode::RENDER_ONLY) {
DCHECK(surface_texture_)

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:201:
UpdateSurfaceTexture(kDoRestoreBindings);
i think that this can be simplified to avoid the mutual recursion.  in the new
world:

this fn handles rendering only, and succeeds silently with kUpdateOnly if it's
already rendered.  UpdateSurfaceTexture takes release_mode, and calls into
ReleaseOutputBuffer, and, depending on the release mode, does the update.

UpdateSurfaceTexture can handle just the context switch and UpdateTexImage call.
 renaming to PromoteBackBuffer or similar would be clearer of this new function.

finally, add a new function UpdateSurface(render_mode, restore_bindings_mode)". 
this function would call Render, then optionally Promote.  it would also own the
code to temporarily attach the surface texture.

all existing calls to UpdateSurfaceTexture and Render would become calls to
UpdateSurface, i think.  default is "do restore bindings", so the outside
doesn't have to care.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:264:
ReleaseOutputBuffer(ReleaseMode::RENDER_AND_UPDATE);
this eats the check for |surface_texture_|.  should still early out, or return
identity or something.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_codec_image.h (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.h:58: enum class ReleaseMode {
SKIP_RENDER, RENDER_ONLY, RENDER_AND_UPDATE };
i think these might be better named { DISCARD_CODEC_BUFFER,
RENDER_TO_BACK_BUFFER, RENDER_TO_FRONT_BUFFER}

RENDER_AND_UPDATE is used for the surfaceview case, but there's no notion of
updating.

[DaleCurtis, 2016-04-22 01:33:12]

Patchset #6 (id:120001) has been deleted

[DaleCurtis, 2016-04-22 01:33:16]

Patchset #5 (id:100001) has been deleted

[DaleCurtis, 2016-04-22 01:33:58]

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:512:
strategy_->MaybeRenderEarly(pictures_out_for_display_);
On 2016/04/22 at 00:51:05, liberato-ooo wrote:
> i see why you'd want to call this periodically, but why twice?

I was thinking that there may be work after DequeueOutput() but after
consideration that case is already covered by SendDecodedFrameToClient().

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:80: const
std::vector<int32_t>& pictures_out_for_display) = 0;
On 2016/04/22 at 00:51:05, liberato-ooo wrote:
> couldn't pictures_out_for_display be an implementation detail of the deferred
strategy, rather than passing it around?  i think that it gets all the callbacks
that it needs to keep it up to date.

Great suggestion. Done.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_codec_image.cc (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:114:
ReleaseOutputBuffer(ReleaseMode::RENDER_ONLY);
On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> RENDER_AND_UPDATE, else it will try to wait for frame available.  or, if you
like my new naming scheme, RENDER_TO_FRONT_BUFFER.

Done.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:176: if (release_mode ==
ReleaseMode::RENDER_ONLY) {
On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> DCHECK(surface_texture_)

Done.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:201:
UpdateSurfaceTexture(kDoRestoreBindings);
On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> i think that this can be simplified to avoid the mutual recursion.  in the new
world:
> 
> this fn handles rendering only, and succeeds silently with kUpdateOnly if it's
already rendered.  UpdateSurfaceTexture takes release_mode, and calls into
ReleaseOutputBuffer, and, depending on the release mode, does the update.
> 
> UpdateSurfaceTexture can handle just the context switch and UpdateTexImage
call.  renaming to PromoteBackBuffer or similar would be clearer of this new
function.
> 
> finally, add a new function UpdateSurface(render_mode,
restore_bindings_mode)".  this function would call Render, then optionally
Promote.  it would also own the code to temporarily attach the surface texture.
> 
> all existing calls to UpdateSurfaceTexture and Render would become calls to
UpdateSurface, i think.  default is "do restore bindings", so the outside
doesn't have to care.

I agree the recursion is nasty, will think on it some more. I don't completely
follow your suggestion though. What is the public AVDACodecImage interface for
the deferred strategy to call when it wants to render to the front buffer now?
It seems odd to leak the bindings mode outside of the image.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:264:
ReleaseOutputBuffer(ReleaseMode::RENDER_AND_UPDATE);
On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> this eats the check for |surface_texture_|.  should still early out, or return
identity or something.

Done.

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_codec_image.h (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.h:58: enum class ReleaseMode {
SKIP_RENDER, RENDER_ONLY, RENDER_AND_UPDATE };
On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> i think these might be better named { DISCARD_CODEC_BUFFER,
RENDER_TO_BACK_BUFFER, RENDER_TO_FRONT_BUFFER}
> 
> RENDER_AND_UPDATE is used for the surfaceview case, but there's no notion of
updating.

I like it. Done.

[liberato (no reviews please), 2016-04-22 02:16:43]

i think the pseudocode is close.  might have missed a few cases.

thanks
-fl

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_codec_image.cc (right):

https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_codec_image.cc:201:
UpdateSurfaceTexture(kDoRestoreBindings);
On 2016/04/22 01:33:58, DaleCurtis wrote:
> On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> > i think that this can be simplified to avoid the mutual recursion.  in the
new
> world:
> > 
> > this fn handles rendering only, and succeeds silently with kUpdateOnly if
it's
> already rendered.  UpdateSurfaceTexture takes release_mode, and calls into
> ReleaseOutputBuffer, and, depending on the release mode, does the update.
> > 
> > UpdateSurfaceTexture can handle just the context switch and UpdateTexImage
> call.  renaming to PromoteBackBuffer or similar would be clearer of this new
> function.
> > 
> > finally, add a new function UpdateSurface(render_mode,
> restore_bindings_mode)".  this function would call Render, then optionally
> Promote.  it would also own the code to temporarily attach the surface
texture.
> > 
> > all existing calls to UpdateSurfaceTexture and Render would become calls to
> UpdateSurface, i think.  default is "do restore bindings", so the outside
> doesn't have to care.
> 
> I agree the recursion is nasty, will think on it some more. I don't completely
> follow your suggestion though. What is the public AVDACodecImage interface for
> the deferred strategy to call when it wants to render to the front buffer now?
> It seems odd to leak the bindings mode outside of the image.

the public api would be UpdateSurface().  i agree about not leaking the bindings
parameter.  that's what i meant about having a default parameter. 
alternatively, public UpdateSurface takes only the renderMode flag, and
protected UpdateSurface takes both.

the deferred strategy would call UpdateSurface(render mode), the same as it does
now with a new name.  UpdateSurface would do something approximately like this:

if (buffer == kInvalidCodecBUffer)
  return;  // success -- copyTexImage gets here!

// move the codec buffer to the surface.
// does nothing if already kUpdateOnly
// switches to kUpdateOnly if ST.
// switches to kInvalidCodecBuffer if SV.
RenderOutputBuffer(render_mode);

if (render_mode != RENDER_TO_FRONT_BUFFER || !surface_texture_)
  return;

if (attached)  {
  PromoteBackBuffer(doRestoreBindings);
  return;
}

// generate fake texture, attach, dontRestoreBindings, detach

}

actually, i guess that it hides the part about restoring bindings too.

[DaleCurtis, 2016-04-22 18:30:55]

On 2016/04/22 at 02:16:43, liberato wrote:
> i think the pseudocode is close.  might have missed a few cases.
> 
> thanks
> -fl
> 
>
https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
> File content/common/gpu/media/avda_codec_image.cc (right):
> 
>
https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
> content/common/gpu/media/avda_codec_image.cc:201:
UpdateSurfaceTexture(kDoRestoreBindings);
> On 2016/04/22 01:33:58, DaleCurtis wrote:
> > On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> > > i think that this can be simplified to avoid the mutual recursion.  in the
new
> > world:
> > > 
> > > this fn handles rendering only, and succeeds silently with kUpdateOnly if
it's
> > already rendered.  UpdateSurfaceTexture takes release_mode, and calls into
> > ReleaseOutputBuffer, and, depending on the release mode, does the update.
> > > 
> > > UpdateSurfaceTexture can handle just the context switch and UpdateTexImage
> > call.  renaming to PromoteBackBuffer or similar would be clearer of this new
> > function.
> > > 
> > > finally, add a new function UpdateSurface(render_mode,
> > restore_bindings_mode)".  this function would call Render, then optionally
> > Promote.  it would also own the code to temporarily attach the surface
texture.
> > > 
> > > all existing calls to UpdateSurfaceTexture and Render would become calls
to
> > UpdateSurface, i think.  default is "do restore bindings", so the outside
> > doesn't have to care.
> > 
> > I agree the recursion is nasty, will think on it some more. I don't
completely
> > follow your suggestion though. What is the public AVDACodecImage interface
for
> > the deferred strategy to call when it wants to render to the front buffer
now?
> > It seems odd to leak the bindings mode outside of the image.
> 
> the public api would be UpdateSurface().  i agree about not leaking the
bindings parameter.  that's what i meant about having a default parameter. 
alternatively, public UpdateSurface takes only the renderMode flag, and
protected UpdateSurface takes both.
> 
> the deferred strategy would call UpdateSurface(render mode), the same as it
does now with a new name.  UpdateSurface would do something approximately like
this:
> 
> if (buffer == kInvalidCodecBUffer)
>   return;  // success -- copyTexImage gets here!
> 
> // move the codec buffer to the surface.
> // does nothing if already kUpdateOnly
> // switches to kUpdateOnly if ST.
> // switches to kInvalidCodecBuffer if SV.
> RenderOutputBuffer(render_mode);
> 
> if (render_mode != RENDER_TO_FRONT_BUFFER || !surface_texture_)
>   return;
> 
> if (attached)  {
>   PromoteBackBuffer(doRestoreBindings);
>   return;
> }
> 
> // generate fake texture, attach, dontRestoreBindings, detach
> 
> }
> 
> actually, i guess that it hides the part about restoring bindings too.

Hmm, CopyTexImage always sends DontRestoreBindings while GetTextureMatrix sends
DoRestoreBindings when attached. Is that a bug?

[liberato (no reviews please), 2016-04-22 18:51:04]

On 2016/04/22 18:30:55, DaleCurtis wrote:
> On 2016/04/22 at 02:16:43, liberato wrote:
> > i think the pseudocode is close.  might have missed a few cases.
> > 
> > thanks
> > -fl
> > 
> >
>
https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
> > File content/common/gpu/media/avda_codec_image.cc (right):
> > 
> >
>
https://codereview.chromium.org/1892013002/diff/80001/content/common/gpu/medi...
> > content/common/gpu/media/avda_codec_image.cc:201:
> UpdateSurfaceTexture(kDoRestoreBindings);
> > On 2016/04/22 01:33:58, DaleCurtis wrote:
> > > On 2016/04/22 at 00:51:06, liberato-ooo wrote:
> > > > i think that this can be simplified to avoid the mutual recursion.  in
the
> new
> > > world:
> > > > 
> > > > this fn handles rendering only, and succeeds silently with kUpdateOnly
if
> it's
> > > already rendered.  UpdateSurfaceTexture takes release_mode, and calls into
> > > ReleaseOutputBuffer, and, depending on the release mode, does the update.
> > > > 
> > > > UpdateSurfaceTexture can handle just the context switch and
UpdateTexImage
> > > call.  renaming to PromoteBackBuffer or similar would be clearer of this
new
> > > function.
> > > > 
> > > > finally, add a new function UpdateSurface(render_mode,
> > > restore_bindings_mode)".  this function would call Render, then optionally
> > > Promote.  it would also own the code to temporarily attach the surface
> texture.
> > > > 
> > > > all existing calls to UpdateSurfaceTexture and Render would become calls
> to
> > > UpdateSurface, i think.  default is "do restore bindings", so the outside
> > > doesn't have to care.
> > > 
> > > I agree the recursion is nasty, will think on it some more. I don't
> completely
> > > follow your suggestion though. What is the public AVDACodecImage interface
> for
> > > the deferred strategy to call when it wants to render to the front buffer
> now?
> > > It seems odd to leak the bindings mode outside of the image.
> > 
> > the public api would be UpdateSurface().  i agree about not leaking the
> bindings parameter.  that's what i meant about having a default parameter. 
> alternatively, public UpdateSurface takes only the renderMode flag, and
> protected UpdateSurface takes both.
> > 
> > the deferred strategy would call UpdateSurface(render mode), the same as it
> does now with a new name.  UpdateSurface would do something approximately like
> this:
> > 
> > if (buffer == kInvalidCodecBUffer)
> >   return;  // success -- copyTexImage gets here!
> > 
> > // move the codec buffer to the surface.
> > // does nothing if already kUpdateOnly
> > // switches to kUpdateOnly if ST.
> > // switches to kInvalidCodecBuffer if SV.
> > RenderOutputBuffer(render_mode);
> > 
> > if (render_mode != RENDER_TO_FRONT_BUFFER || !surface_texture_)
> >   return;
> > 
> > if (attached)  {
> >   PromoteBackBuffer(doRestoreBindings);
> >   return;
> > }
> > 
> > // generate fake texture, attach, dontRestoreBindings, detach
> > 
> > }
> > 
> > actually, i guess that it hides the part about restoring bindings too.
> 
> Hmm, CopyTexImage always sends DontRestoreBindings while GetTextureMatrix
sends
> DoRestoreBindings when attached. Is that a bug?

no, not a bug in the existing code.  just broken pseudocode, sorry, good catch. 
that's also why the bindings parameter disappeared, mistakenly, from
UpdateSurface.

here are the cases:

copyTexImage: enforces that the surface texture's client texture is already
current before calling UpdateSurfaceTexture, so uses dontRestore to save the
glGets / possible pipeline stalls since no action is needed.
GetTextureMatrix: uses doRestore if attached, since it has no idea if the GL
state matches the one needed for st.
GetTextureMatrix: uses dontRestore if not attached, since it had to do the glGet
before doing the attach anyway.  no need to do another in UpdateSurfaceTexture.

the new case you're adding here would also doRestore.  might be able to get away
with dontRestore in some cases, but there are too many call sites to do this
safely.

the pseudocode as written misses the copyTexImage optimization that can be
applied in the attached case. sorry about that.

[DaleCurtis, 2016-04-22 19:28:04]

Okay, I think I got all the cases covered. Please give it your best eyes!

[liberato (no reviews please), 2016-04-22 20:25:09]

lgtm % nits + (potential) iterator erase issue.  erasing just 
|id| is probably sufficient.

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:228:
std::remove(pictures_out_for_display_.begin(),
i don't understand what this does.  std::remove() will return an iterator to
something that is not |id|.  ::erase will erase it.

also not sure why erasing to the end is a good idea.  isn't that the newest
frame?  wouldn't one want to erase from the beginning?  would one want to erase
a whole range?  i actually think that one would not.

it's fairly common that i get stl iterators wrong, though, so maybe i'm just
doing that here too.

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.cc (right):

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:183: if (!surface_texture_) {
DCHECK_NE(...BACK_BUFFER)?

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:184: codec_buffer_index_ =
kInvalidCodecBufferIndex;
this might be better in ReleaseOutputBuffer.

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.h (right):

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.h:63: // which turns the back buffer
into the front.
this comment isn't right for SV.  we use FRONT_BUFFER but we don't call
UpdateTexImage.

in the SV case, we're technically rendering to a back buffer, but surfaceflinger
will promote it automatically for us.  i still like the name TO_FRONT_BUFFER for
this case, since that's where it ends up after this call returns, just
asynchronously.

[DaleCurtis, 2016-04-22 20:58:02]

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:228:
std::remove(pictures_out_for_display_.begin(),
On 2016/04/22 at 20:25:08, liberato wrote:
> i don't understand what this does.  std::remove() will return an iterator to
something that is not |id|.  ::erase will erase it.
> 
> also not sure why erasing to the end is a good idea.  isn't that the newest
frame?  wouldn't one want to erase from the beginning?  would one want to erase
a whole range?  i actually think that one would not.
> 
> it's fairly common that i get stl iterators wrong, though, so maybe i'm just
doing that here too.

https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom

std::remove() rotates the element(s) to be erased to the end of the vector and
then .erase() removes the end of the range (which is all to-be-deleted elements)

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.cc (right):

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:183: if (!surface_texture_) {
On 2016/04/22 at 20:25:08, liberato wrote:
> DCHECK_NE(...BACK_BUFFER)?

Done, though can't use DCHECK_NE with enum class for some reason.

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.cc:184: codec_buffer_index_ =
kInvalidCodecBufferIndex;
On 2016/04/22 at 20:25:08, liberato wrote:
> this might be better in ReleaseOutputBuffer.

Good point, done.

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/avda_codec_image.h (right):

https://codereview.chromium.org/1892013002/diff/160001/content/common/gpu/med...
content/common/gpu/media/avda_codec_image.h:63: // which turns the back buffer
into the front.
On 2016/04/22 at 20:25:08, liberato wrote:
> this comment isn't right for SV.  we use FRONT_BUFFER but we don't call
UpdateTexImage.
> 
> in the SV case, we're technically rendering to a back buffer, but
surfaceflinger will promote it automatically for us.  i still like the name
TO_FRONT_BUFFER for this case, since that's where it ends up after this call
returns, just asynchronously.

Comment clarified.

[watk, 2016-04-22 23:17:48]

awesome, lgtm

[DaleCurtis, 2016-04-23 02:09:45]

Thanks for review. Unfortunately this isn't sufficient to fix all the moto x
problems, 720p60 is still looking rough, but 720p30 is fixed with this, so
CQ'ing!

[DaleCurtis, 2016-04-23 02:09:49]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-04-23 02:09:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, watk@chromium.org
Link to the patchset: https://codereview.chromium.org/1892013002/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-04-23 02:10:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892013002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892013002/200001

[commit-bot: I haz the power, 2016-04-23 03:02:16]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-04-23 03:03:52]

Description was changed from

==========
ReleaseOutputBuffer to surface back and front buffers where possible.

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

This proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for some more details on the off screen issue.

This patch unifies all calls from the deferred strategy for releasing
output buffers into a single AVDACodecImage::ReleaseOutputBuffer() call
with a couple modes to support all the cases we use:
- skip render: ReleaseOutputBuffer(, false);
- release only: ReleaseOutputBuffer(, true); FrameListener.Wait();
- release+update: same as "release only" but also UpdateTexImage().

BUG=601066
TEST=manual testing on n7v2 and moto x.
==========

to

==========
ReleaseOutputBuffer to surface back and front buffers where possible.

Takes the original patch from liberato@ and extends it to release the
earliest frame to the back buffer as well as releasing to the front buffer
when there is only a single outstanding frame for display.

This proves enough to solve the slow n7v2 issue as well as keep decoding
up to speed when scrolling the video off the page on an motox, see
http://crbug.com/603768 for some more details on the off screen issue.

This patch unifies all calls from the deferred strategy for releasing
output buffers into a single AVDACodecImage::ReleaseOutputBuffer() call
with a couple modes to support all the cases we use:
- skip render: ReleaseOutputBuffer(, false);
- release only: ReleaseOutputBuffer(, true); FrameListener.Wait();
- release+update: same as "release only" but also UpdateTexImage().

BUG=601066
TEST=manual testing on n7v2 and moto x.

Committed: https://crrev.com/1ee5b842e11f679b21a38f156fb7d4688ff8665a
Cr-Commit-Position: refs/heads/master@{#389350}
==========

[commit-bot: I haz the power, 2016-04-23 03:03:54]

Patchset 8 (id:??) landed as
https://crrev.com/1ee5b842e11f679b21a38f156fb7d4688ff8665a
Cr-Commit-Position: refs/heads/master@{#389350}