Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

This dramatically improves the fullscreen transition; it's much
faster and does not cause audio jank.

BUG=662251
TEST=enter/exit fullscreen on M+ device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/4b632fce277d847aa2c5a533478da715086c0b55
Cr-Commit-Position: refs/heads/master@{#431114}

[DaleCurtis, 2016-10-28 21:34:13]

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

[DaleCurtis, 2016-10-28 21:34:13]

Is missing support for a couple edge cases, but the idea is wholesome!

  // TODO(dalecurtis): What to do about AllocateSurface()?
  // TODO(dalecurtis): What to do about in flight configurations?

[DaleCurtis, 2016-10-28 22:01:01]

Not entirely wholesome haha, forgot to send the IPC message over to actually
call SetSurface, was testing SurfaceTexture :| When wired it now explodes :)
Will update when resolved.

[DaleCurtis, 2016-10-28 22:29:29]

http://pastebin.com/3frHyTq7 has the failure, will take another look next week.

[liberato (no reviews please), 2016-10-29 18:57:20]

i've thought about doing something similar several times, but always talked
myself out of it.  probably a mistake on my part.

haven't looked in great detail yet.  just wanted to toss an idea out.

i'm not sure that the ST -> SV -> ST transition is being handled properly by the
codec images.  first, i don't remember what happens to unrendered codec buffers
on setOutputSurface -- do they go with the output surface that was current when
we got them out of the codec, or do they transition to the new one?

if the former, then we might need to copy surface texture out of the shared
state into the codec image when we UseCodecBufferForPicture.  that way, it goes
with the buffer instead of the global state.

even if the latter, the front buffer of the ST / SV might need to be handled
specially.  that one definitely doesn't transition on SetSurface.  then again,
it might be fine since AVDACodecImage will take no action if it's already
rendered.

i need to look more closely, though.

-fl

https://codereview.chromium.org/2461073002/diff/20001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/20001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:340: // If initialization has already
completed, then send the new surface id.
this is a little confusing.  it might be clearer if AVDA always defers init
until the first SetSurface() call instead.  that would also let us combine the
"codec config is in flight" case with "codec config hasn't started yet".  
something like:

// remove surface_id from VDA::Config.

AVDA::SetSurface() {
  if (WAITING_FOR_CODEC) {
    // we're restarting the codec for some reason.
    pending_surface_ = surface_id_;
    // OnCodecConfigured will notice and SetSurface.
    return;
  } else if (!codec) {
    // initial setup of VDA -- complete init now.
    set surface_id_ in codec_config_ and start async config;
  } else {
    codec->setSurface()
  }

then, we don't have two paths where the surface id is sent in, and it handles
one of the edge cases you mention in AVDA::SetSurface.

if the config specifies that deferred init is not possible, then AVDA might need
to call AVDA::SetSurface(kNoSurfaceId).  that's a little hacky, of a similar
type to what i'm trying to remove here.  :)

gvd would go back to doing the CompleteInitialization() work immediately.

i think that this also helps us make the AllocateSurface case not special.  it
would move out of AVDA::Initialize into setSurface, i think.  AllocateSurface
doesn't handle the "already own the codec" case correctly -- it should succeed
immediately as it does if there is no current owner.

https://codereview.chromium.org/2461073002/diff/20001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2461073002/diff/20001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:1249: // TODO(dalecurtis): What to
do about AllocateSurface()?
since we didn't restart, i think that there should never be a waiter.  i'll
think about it more.

however, if we make SetSurface a normal part of initialization, then it's not an
edge case anyway.

[liberato (no reviews please), 2016-10-30 06:19:31]

On 2016/10/29 18:57:20, liberato wrote:
> i've thought about doing something similar several times, but always talked
> myself out of it.  probably a mistake on my part.
> 
> haven't looked in great detail yet.  just wanted to toss an idea out.
> 
> i'm not sure that the ST -> SV -> ST transition is being handled properly by
the
> codec images.  first, i don't remember what happens to unrendered codec
buffers
> on setOutputSurface -- do they go with the output surface that was current
when
> we got them out of the codec, or do they transition to the new one?
> 
> if the former, then we might need to copy surface texture out of the shared
> state into the codec image when we UseCodecBufferForPicture.  that way, it
goes
> with the buffer instead of the global state.
> 
> even if the latter, the front buffer of the ST / SV might need to be handled
> specially.  that one definitely doesn't transition on SetSurface.  then again,
> it might be fine since AVDACodecImage will take no action if it's already
> rendered.
> 
> i need to look more closely, though.
> 
> -fl
> 
>
https://codereview.chromium.org/2461073002/diff/20001/media/filters/gpu_video...
> File media/filters/gpu_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/20001/media/filters/gpu_video...
> media/filters/gpu_video_decoder.cc:340: // If initialization has already
> completed, then send the new surface id.
> this is a little confusing.  it might be clearer if AVDA always defers init
> until the first SetSurface() call instead.  that would also let us combine the
> "codec config is in flight" case with "codec config hasn't started yet".  
> something like:
> 
> // remove surface_id from VDA::Config.
> 
> AVDA::SetSurface() {
>   if (WAITING_FOR_CODEC) {
>     // we're restarting the codec for some reason.
>     pending_surface_ = surface_id_;
>     // OnCodecConfigured will notice and SetSurface.
>     return;
>   } else if (!codec) {
>     // initial setup of VDA -- complete init now.
>     set surface_id_ in codec_config_ and start async config;
>   } else {
>     codec->setSurface()
>   }
> 
> then, we don't have two paths where the surface id is sent in, and it handles
> one of the edge cases you mention in AVDA::SetSurface.
> 
> if the config specifies that deferred init is not possible, then AVDA might
need
> to call AVDA::SetSurface(kNoSurfaceId).  that's a little hacky, of a similar
> type to what i'm trying to remove here.  :)
> 
> gvd would go back to doing the CompleteInitialization() work immediately.
> 
> i think that this also helps us make the AllocateSurface case not special.  it
> would move out of AVDA::Initialize into setSurface, i think.  AllocateSurface
> doesn't handle the "already own the codec" case correctly -- it should succeed
> immediately as it does if there is no current owner.
> 
>
https://codereview.chromium.org/2461073002/diff/20001/media/gpu/android_video...
> File media/gpu/android_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/20001/media/gpu/android_video...
> media/gpu/android_video_decode_accelerator.cc:1249: // TODO(dalecurtis): What
to
> do about AllocateSurface()?
> since we didn't restart, i think that there should never be a waiter.  i'll
> think about it more.
> 
> however, if we make SetSurface a normal part of initialization, then it's not
an
> edge case anyway.

here's the magic bit of setSurface that handles re-parenting the graphic buffers
between surfaces, i think:

https://android.googlesource.com/platform/frameworks/av/+/android-cts-7.0_r2/...

still tracking down exactly how the state machine works, but i think that the
next codec buffer released for rendering will go to the new surface, regardless
of what surface was set when it was dequeued from MediaCodec by AVDA.  not at
all sure about 'metadata mode'.  never heard of it before...

after more thought, i'm still feeling pretty good about this approach to
setSurface working.  nifty!

[DaleCurtis, 2016-10-31 22:05:57]

Latest patch set gets the transition to fullscreen working by waiting for all
outstanding frames to be rendered and then calling setSurface() to ensure we
mark frames as overlayable properly.

Unfortunately for the transition back to SurfaceTexture we don't have that
liberty, the surface is destroyed along with ContentVideoView throwing all sorts
of codec errors. I'm tinkering with the sync IPC watk@ added a while back now,
but otherwise we may need to wait for DialogSurface after all to do this
reasonably :/

[DaleCurtis, 2016-10-31 22:57:53]

Okay entrance and exit work now, but it's messy. Here's why:
1. We specify texture target and size during the original picture buffer
request. To prevent flickering in dev tools we send either 2D (SV) or OES (ST).
2. Creating a new SurfaceTexture doesn't seem to work. We must reuse the
originally created SurfaceTexture otherwise the exit from fullscreen never
returns another frame.
3. Since we have no time to wait for outstanding buffers, we must force a "codec
change" to invalidate all outstanding frames during the synchronous surface
destruction callback.
4. Due to 3 occurring before the renderer can send a SetSurface(), we must
ignore duplicate SetSurface requests to AVDA.

1, 2, 3, 4 would probably all go away with the presence of VDAv2 +
DialogSurface. I think this can be cleaned up and made working, but will regress
the dev tools flickering issue.

[liberato (no reviews please), 2016-11-01 14:36:59]

On 2016/10/31 22:57:53, DaleCurtis wrote:
> Okay entrance and exit work now, but it's messy. Here's why:
> 1. We specify texture target and size during the original picture buffer
> request. To prevent flickering in dev tools we send either 2D (SV) or OES
(ST).
> 2. Creating a new SurfaceTexture doesn't seem to work. We must reuse the
> originally created SurfaceTexture otherwise the exit from fullscreen never
> returns another frame.
> 3. Since we have no time to wait for outstanding buffers, we must force a
"codec
> change" to invalidate all outstanding frames during the synchronous surface
> destruction callback.
> 4. Due to 3 occurring before the renderer can send a SetSurface(), we must
> ignore duplicate SetSurface requests to AVDA.
> 
> 1, 2, 3, 4 would probably all go away with the presence of VDAv2 +
> DialogSurface. I think this can be cleaned up and made working, but will
regress
> the dev tools flickering issue.

i agree that 1,3,4 should all go away with DS+VDAv2.

2, however, is entirely unexpected.  do you mean that the codec just stops
returning decoded frames, or something later in the pipe quits working?

might it be easier to improve the "enter full screen" transition, but still
unconditionally restart the codec on exit in WMPI::DisableOverlay?  'enter'
seems much more important, anyway.  it would avoid 2, 3, 4.

[DaleCurtis, 2016-11-01 20:45:17]

I'm guessing #2 is actually just a bug in my implementation somewhere. I'm not
setting the texture correctly perhaps.

[DaleCurtis, 2016-11-01 20:46:47]

On 2016/11/01 at 20:45:17, DaleCurtis wrote:
> I'm guessing #2 is actually just a bug in my implementation somewhere. I'm not
setting the texture correctly perhaps.

Also I mean the decoder keeps returning frames, but the picture never changes
again. I think we should try to avoid restarting the codec if possible. #3,#4
aren't all that bad in any case.

[liberato (no reviews please), 2016-11-01 20:57:17]

personally, i think that getting rid of the surface texture is a good idea,
since it'll have all the back buffers allocated.

https://codereview.chromium.org/2461073002/diff/20001/media/gpu/avda_picture_...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.cc:171: on_frame_available_handler_ = new
OnFrameAvailableHandler(
it was probably using the old |service_id| in the GLImages.  when the codec
images are attached in AVDAPictureBufferManager::SetImageForPicture, the call to
SetLevelStreamTextureImage provides |service_id|.  One can update it later by
calling Texture::SetStreamTextureServiceId.  it should be safe to do so here.

also, one must be sure to delete the outgoing |service_id| after that.  since
the last frame was still visible, i suspect that it wasn't being deleted. 
~SharedState takes care of this normally.

[DaleCurtis, 2016-11-03 19:30:18]

Description was changed from

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

BUG=tbd
TEST=manual
==========

to

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

BUG=tbd
TEST=manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2016-11-03 19:32:36]

This relocates quite a bit of code that I'll split out into smaller CLs before
landing; but please look over the idea and let me know if this is sane now.

https://codereview.chromium.org/2461073002/diff/20001/media/gpu/avda_picture_...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/20001/media/gpu/avda_picture_...
media/gpu/avda_picture_buffer_manager.cc:171: on_frame_available_handler_ = new
OnFrameAvailableHandler(
On 2016/11/01 at 20:57:16, liberato wrote:
> it was probably using the old |service_id| in the GLImages.  when the codec
images are attached in AVDAPictureBufferManager::SetImageForPicture, the call to
SetLevelStreamTextureImage provides |service_id|.  One can update it later by
calling Texture::SetStreamTextureServiceId.  it should be safe to do so here.
> 
> also, one must be sure to delete the outgoing |service_id| after that.  since
the last frame was still visible, i suspect that it wasn't being deleted. 
~SharedState takes care of this normally.

Thanks for this! The latest patch set uses this method, but needed to make that
method public first.

[watk, 2016-11-03 20:26:34]

nice! idea looks good

https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
content/browser/media/android/browser_surface_view_manager.cc:113:
base::android::BuildInfo::GetInstance()->sdk_int() < 23) {
We might want to just do this always now. I restricted it initially to see if it
caused crashes. But if it's enabled for a decent % of devices I reckon we should
enable it for all devices because it will let us remove the deferred error logic
we have in AVDA.

Actually, do you know where we can find ANR rates to check how this is going on
JB? I assume that's how deadlocks would manifest. Other kinds of failures should
result in crash reports at least.

https://codereview.chromium.org/2461073002/diff/100001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461073002/diff/100001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:631:
Log.e(TAG, "Cannot set output surface", e);
minor: you should be able to write "catch(IAE | ISE e) {" to handle them both in
one block.

https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1473: void
WebMediaPlayerImpl::OnSurfaceRequested(
Naming suggestion for when the time comes. This feels more like
"SetSurfaceChangeObserver" or something like that to me now. i.e., the decoder
is an observer of surface changes rather than a requester of surfaces.

This changed a while back when we started requesting the surface on fullscreen
transitions instead of in response to this call.

https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:344: return;
Not sure how this is hit?

[liberato (no reviews please), 2016-11-03 20:51:04]

looks pretty solid.  my only concern is the commented out service_id check in
CopyTexImage.

-fl

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1348: // picture buffers to avoid
putting the GL system in a broken state.
we can almost un-overlay these things in the overlay candidate validator, except
that we'd have to ask GLStreamTextureImage a question.  that's likely a
performance problem, unfortunately.

if we could, somehow, fail the overlay promotion, then it would draw in the
normal path.

i'll give it some thought.  it'll probably turn out not to be worth the effort,
since DS will avoid it altogether.

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1356: if (pending_surface_id_) {
if it comes in later, then we might already be using surface texture.  in that
case, SetSurface should probably notice that we're already using surface texture
and not set |pending_surface_id_|.  otherwise, DequeueOutput will wait for
outstanding buffers.

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
media/gpu/avda_codec_image.cc:55: // GLint bound_service_id = 0;
now that we're swapping out |shared_state_| when setting up a picture for a new
frame, how does it get into this situation?

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:230: // always set BOUND for SV.
we should do that here too, i think.

actually, i think that one can use SetLevelStreamTextureForImage, or (maybe)
just call SetImageForPicture() again here.  didn't check that there aren't other
side-effects, but i don't think that there are.

[DaleCurtis, 2016-11-03 20:59:36]

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:230: // always set BOUND for SV.
On 2016/11/03 at 20:51:04, liberato wrote:
> we should do that here too, i think.
> 
> actually, i think that one can use SetLevelStreamTextureForImage, or (maybe)
just call SetImageForPicture() again here.  didn't check that there aren't other
side-effects, but i don't think that there are.

Originally tried this, but there are some restrictions I haven't sorted out
about calling SetImageForPicture() again. It's possibly something silly, but
haven't looked closely. It fails with the following:

[FATAL:ref_counted.h(52)] Check failed: !in_dtor_. 

Stack Trace:
  RELADDR   FUNCTION                                               FILE:LINE
  00096fa3  ~LogMessage                                           
/d/code/chrome/src/base/logging.cc:532
  v------>  AddRef                                                
/d/code/chrome/src/base/memory/ref_counted.h:52
  v------>  AddRef                                                
/d/code/chrome/src/base/memory/ref_counted.h:130
  v------>  AddRef                                                
/d/code/chrome/src/base/memory/ref_counted.h:400
  0008d51f  operator=                                             
/d/code/chrome/src/base/memory/ref_counted.h:326
  000f3dcb  SetLevelImageInternal                                 
/d/code/chrome/src/gpu/command_buffer/service/texture_manager.cc:1617
  v------>  SetLevelStreamTextureImage                            
/d/code/chrome/src/gpu/command_buffer/service/texture_manager.cc:1640
  000f5f4f  SetLevelStreamTextureImage                            
/d/code/chrome/src/gpu/command_buffer/service/texture_manager.cc:2238
  00015c67  SetImageForPicture                                    
/d/code/chrome/src/media/gpu/avda_picture_buffer_manager.cc:193
  00015e23  UseCodecBufferForPictureBuffer                        
/d/code/chrome/src/media/gpu/avda_picture_buffer_manager.cc:222
  00010277  SendDecodedFrameToClient                              
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:855
  0000f729  DequeueOutput                                         
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:782
  0000e907  DoIOTask                                              
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:501
  00012d31  RunTimer                                              
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:247
  v------>  Run                                                   
/d/code/chrome/src/base/callback.h:64
  000e54e5  RunScheduledTask                                      
/d/code/chrome/src/base/timer/timer.cc:213
  v------>  Run                                                   
/d/code/chrome/src/base/callback.h:47
  000815df  RunTask                                               
/d/code/chrome/src/base/debug/task_annotator.cc:52
  0009eb41  RunTask                                               
/d/code/chrome/src/base/message_loop/message_loop.cc:413
  0009eda3  DeferOrRunPendingTask                                 
/d/code/chrome/src/base/message_loop/message_loop.cc:422
  0009f1a1  DoDelayedWork                                         
/d/code/chrome/src/base/message_loop/message_loop.cc:554
  000a1405  Run                                                   
/d/code/chrome/src/base/message_loop/message_pump_libevent.cc:228
  0009e8cd  RunHandler                                            
/d/code/chrome/src/base/message_loop/message_loop.cc:378
  000baab1  Run                                                   
/d/code/chrome/src/base/run_loop.cc:35
  003efddb  GpuMain                                               
/d/code/chrome/src/content/gpu/gpu_main.cc:288
  009865e9  Run                                                   
/d/code/chrome/src/content/app/content_main_runner.cc:776
  v------>  Start                                                 
/d/code/chrome/src/content/app/android/content_main.cc:46
  00985909  Java_org_chromium_content_app_ContentMain_nativeStart 
/d/code/chrome/src/out_android/Release/gen/content/public/android/content_jni_headers/content/jni/ContentMain_jni.h:38
  000855a5  <unknown>                                             
/data/data/org.chromium.chrome/incremental-install-files/optimized-dexes/content.public.android.content_java.dex.dex

[DaleCurtis, 2016-11-03 23:25:06]

https://codereview.chromium.org/2461073002/diff/100001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461073002/diff/100001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:631:
Log.e(TAG, "Cannot set output surface", e);
On 2016/11/03 at 20:26:34, watk wrote:
> minor: you should be able to write "catch(IAE | ISE e) {" to handle them both
in one block.

Done.

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:230: // always set BOUND for SV.
On 2016/11/03 at 20:59:35, DaleCurtis wrote:
> On 2016/11/03 at 20:51:04, liberato wrote:
> > we should do that here too, i think.
> > 
> > actually, i think that one can use SetLevelStreamTextureForImage, or (maybe)
just call SetImageForPicture() again here.  didn't check that there aren't other
side-effects, but i don't think that there are.
> 
> Originally tried this, but there are some restrictions I haven't sorted out
about calling SetImageForPicture() again. It's possibly something silly, but
haven't looked closely. It fails with the following:
> 
> [FATAL:ref_counted.h(52)] Check failed: !in_dtor_. 
> 
> Stack Trace:
>   RELADDR   FUNCTION                                               FILE:LINE
>   00096fa3  ~LogMessage                                           
/d/code/chrome/src/base/logging.cc:532
>   v------>  AddRef                                                
/d/code/chrome/src/base/memory/ref_counted.h:52
>   v------>  AddRef                                                
/d/code/chrome/src/base/memory/ref_counted.h:130
>   v------>  AddRef                                                
/d/code/chrome/src/base/memory/ref_counted.h:400
>   0008d51f  operator=                                             
/d/code/chrome/src/base/memory/ref_counted.h:326
>   000f3dcb  SetLevelImageInternal                                 
/d/code/chrome/src/gpu/command_buffer/service/texture_manager.cc:1617
>   v------>  SetLevelStreamTextureImage                            
/d/code/chrome/src/gpu/command_buffer/service/texture_manager.cc:1640
>   000f5f4f  SetLevelStreamTextureImage                            
/d/code/chrome/src/gpu/command_buffer/service/texture_manager.cc:2238
>   00015c67  SetImageForPicture                                    
/d/code/chrome/src/media/gpu/avda_picture_buffer_manager.cc:193
>   00015e23  UseCodecBufferForPictureBuffer                        
/d/code/chrome/src/media/gpu/avda_picture_buffer_manager.cc:222
>   00010277  SendDecodedFrameToClient                              
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:855
>   0000f729  DequeueOutput                                         
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:782
>   0000e907  DoIOTask                                              
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:501
>   00012d31  RunTimer                                              
/d/code/chrome/src/media/gpu/android_video_decode_accelerator.cc:247
>   v------>  Run                                                   
/d/code/chrome/src/base/callback.h:64
>   000e54e5  RunScheduledTask                                      
/d/code/chrome/src/base/timer/timer.cc:213
>   v------>  Run                                                   
/d/code/chrome/src/base/callback.h:47
>   000815df  RunTask                                               
/d/code/chrome/src/base/debug/task_annotator.cc:52
>   0009eb41  RunTask                                               
/d/code/chrome/src/base/message_loop/message_loop.cc:413
>   0009eda3  DeferOrRunPendingTask                                 
/d/code/chrome/src/base/message_loop/message_loop.cc:422
>   0009f1a1  DoDelayedWork                                         
/d/code/chrome/src/base/message_loop/message_loop.cc:554
>   000a1405  Run                                                   
/d/code/chrome/src/base/message_loop/message_pump_libevent.cc:228
>   0009e8cd  RunHandler                                            
/d/code/chrome/src/base/message_loop/message_loop.cc:378
>   000baab1  Run                                                   
/d/code/chrome/src/base/run_loop.cc:35
>   003efddb  GpuMain                                               
/d/code/chrome/src/content/gpu/gpu_main.cc:288
>   009865e9  Run                                                   
/d/code/chrome/src/content/app/content_main_runner.cc:776
>   v------>  Start                                                 
/d/code/chrome/src/content/app/android/content_main.cc:46
>   00985909  Java_org_chromium_content_app_ContentMain_nativeStart 
/d/code/chrome/src/out_android/Release/gen/content/public/android/content_jni_headers/content/jni/ContentMain_jni.h:38
>   000855a5  <unknown>                                             
/data/data/org.chromium.chrome/incremental-install-files/optimized-dexes/content.public.android.content_java.dex.dex

Ah, this was because the last ref to the GLImage was being overwritten by
something which didn't hold a ref. Fixed, but now seeing some errors like this
that I'm tracking down:

11-03 16:21:02.521 31028 31068 W Adreno-ES20: <core_glTexSubImage2D:554>:
GL_INVALID_ENUM
11-03 16:21:02.522 31028 31068 E chromium: [ERROR:gles2_cmd_decoder.cc(2402)]
[.DisplayCompositor-0x9e76a000]GL ERROR :GL_INVALID_ENUM :
GLES2DecoderImpl::PrepareTexturesForRender: <- error from previous GL command

[DaleCurtis, 2016-11-04 01:02:46]

Description was changed from

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

BUG=tbd
TEST=manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

This dramatically improves the fullscreen transition; it's much
faster and does not cause audio jank.

BUG=662251
TEST=enter/exit fullscreen on M+ device.
==========

[DaleCurtis, 2016-11-04 01:07:22]

Latest patch set resolves almost all comments and is rebased relative to the
cleanup CL so it should be easier to look at now. The only unresolved issue are
these errors which spam on first display of the ST frames:

11-03 16:21:02.521 31028 31068 W Adreno-ES20: <core_glTexSubImage2D:554>:
GL_INVALID_ENUM
11-03 16:21:02.522 31028 31068 E chromium: [ERROR:gles2_cmd_decoder.cc(2402)]
[.DisplayCompositor-0x9e76a000]GL ERROR :GL_INVALID_ENUM :
GLES2DecoderImpl::PrepareTexturesForRender: <- error from previous GL command

PrepareTexturesForRender() suppresses errors from this stage so nothing goes
wrong, but I'd like to understand why they're showing up.

https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
content/browser/media/android/browser_surface_view_manager.cc:113:
base::android::BuildInfo::GetInstance()->sdk_int() < 23) {
On 2016/11/03 at 20:26:34, watk wrote:
> We might want to just do this always now. I restricted it initially to see if
it caused crashes. But if it's enabled for a decent % of devices I reckon we
should enable it for all devices because it will let us remove the deferred
error logic we have in AVDA.
> 
> Actually, do you know where we can find ANR rates to check how this is going
on JB? I assume that's how deadlocks would manifest. Other kinds of failures
should result in crash reports at least.

Probably you can use crash/blame with the file path to this point and see what
pops up. I can poke around tomorrow.

https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1473: void
WebMediaPlayerImpl::OnSurfaceRequested(
On 2016/11/03 at 20:26:34, watk wrote:
> Naming suggestion for when the time comes. This feels more like
"SetSurfaceChangeObserver" or something like that to me now. i.e., the decoder
is an observer of surface changes rather than a requester of surfaces.
> 
> This changed a while back when we started requesting the surface on fullscreen
transitions instead of in response to this call.

Agreed I don't like the name, but since the mechanism is double duty depending
on platform support it's not always an observer. Will see if I can come up with
a better name.

https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:344: return;
On 2016/11/03 at 20:26:34, watk wrote:
> Not sure how this is hit?

Heh, without this we can't change surfaces :) -- that always happens post-init.

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1356: if (pending_surface_id_) {
On 2016/11/03 at 20:51:04, liberato wrote:
> if it comes in later, then we might already be using surface texture.  in that
case, SetSurface should probably notice that we're already using surface texture
and not set |pending_surface_id_|.  otherwise, DequeueOutput will wait for
outstanding buffers.

This case is already handled in SetSurface and here?

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
media/gpu/avda_codec_image.cc:55: // GLint bound_service_id = 0;
On 2016/11/03 at 20:51:04, liberato wrote:
> now that we're swapping out |shared_state_| when setting up a picture for a
new frame, how does it get into this situation?

Just left over testing. Removed.

[liberato (no reviews please), 2016-11-07 19:42:50]

On 2016/11/04 01:07:22, DaleCurtis wrote:
> Latest patch set resolves almost all comments and is rebased relative to the
> cleanup CL so it should be easier to look at now. The only unresolved issue
are
> these errors which spam on first display of the ST frames:
> 
> 11-03 16:21:02.521 31028 31068 W Adreno-ES20: <core_glTexSubImage2D:554>:
> GL_INVALID_ENUM
> 11-03 16:21:02.522 31028 31068 E chromium: [ERROR:gles2_cmd_decoder.cc(2402)]
> [.DisplayCompositor-0x9e76a000]GL ERROR :GL_INVALID_ENUM :
> GLES2DecoderImpl::PrepareTexturesForRender: <- error from previous GL command
> 
> PrepareTexturesForRender() suppresses errors from this stage so nothing goes
> wrong, but I'd like to understand why they're showing up.
> 
>
https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
> File content/browser/media/android/browser_surface_view_manager.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
> content/browser/media/android/browser_surface_view_manager.cc:113:
> base::android::BuildInfo::GetInstance()->sdk_int() < 23) {
> On 2016/11/03 at 20:26:34, watk wrote:
> > We might want to just do this always now. I restricted it initially to see
if
> it caused crashes. But if it's enabled for a decent % of devices I reckon we
> should enable it for all devices because it will let us remove the deferred
> error logic we have in AVDA.
> > 
> > Actually, do you know where we can find ANR rates to check how this is going
> on JB? I assume that's how deadlocks would manifest. Other kinds of failures
> should result in crash reports at least.
> 
> Probably you can use crash/blame with the file path to this point and see what
> pops up. I can poke around tomorrow.
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:1473: void
> WebMediaPlayerImpl::OnSurfaceRequested(
> On 2016/11/03 at 20:26:34, watk wrote:
> > Naming suggestion for when the time comes. This feels more like
> "SetSurfaceChangeObserver" or something like that to me now. i.e., the decoder
> is an observer of surface changes rather than a requester of surfaces.
> > 
> > This changed a while back when we started requesting the surface on
fullscreen
> transitions instead of in response to this call.
> 
> Agreed I don't like the name, but since the mechanism is double duty depending
> on platform support it's not always an observer. Will see if I can come up
with
> a better name.
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
> File media/filters/gpu_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
> media/filters/gpu_video_decoder.cc:344: return;
> On 2016/11/03 at 20:26:34, watk wrote:
> > Not sure how this is hit?
> 
> Heh, without this we can't change surfaces :) -- that always happens
post-init.
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
> File media/gpu/android_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
> media/gpu/android_video_decode_accelerator.cc:1356: if (pending_surface_id_) {
> On 2016/11/03 at 20:51:04, liberato wrote:
> > if it comes in later, then we might already be using surface texture.  in
that
> case, SetSurface should probably notice that we're already using surface
texture
> and not set |pending_surface_id_|.  otherwise, DequeueOutput will wait for
> outstanding buffers.
> 
> This case is already handled in SetSurface and here?
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
> File media/gpu/avda_codec_image.cc (right):
> 
>
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
> media/gpu/avda_codec_image.cc:55: // GLint bound_service_id = 0;
> On 2016/11/03 at 20:51:04, liberato wrote:
> > now that we're swapping out |shared_state_| when setting up a picture for a
> new frame, how does it get into this situation?
> 
> Just left over testing. Removed.

do the gl errors show up immediately, or only after a full screen => embedded
transition?

[DaleCurtis, 2016-11-07 19:45:04]

On 2016/11/07 at 19:42:50, liberato wrote:
> On 2016/11/04 01:07:22, DaleCurtis wrote:
> > Latest patch set resolves almost all comments and is rebased relative to the
> > cleanup CL so it should be easier to look at now. The only unresolved issue
are
> > these errors which spam on first display of the ST frames:
> > 
> > 11-03 16:21:02.521 31028 31068 W Adreno-ES20: <core_glTexSubImage2D:554>:
> > GL_INVALID_ENUM
> > 11-03 16:21:02.522 31028 31068 E chromium:
[ERROR:gles2_cmd_decoder.cc(2402)]
> > [.DisplayCompositor-0x9e76a000]GL ERROR :GL_INVALID_ENUM :
> > GLES2DecoderImpl::PrepareTexturesForRender: <- error from previous GL
command
> > 
> > PrepareTexturesForRender() suppresses errors from this stage so nothing goes
> > wrong, but I'd like to understand why they're showing up.
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
> > File content/browser/media/android/browser_surface_view_manager.cc (right):
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/content/browser/media/...
> > content/browser/media/android/browser_surface_view_manager.cc:113:
> > base::android::BuildInfo::GetInstance()->sdk_int() < 23) {
> > On 2016/11/03 at 20:26:34, watk wrote:
> > > We might want to just do this always now. I restricted it initially to see
if
> > it caused crashes. But if it's enabled for a decent % of devices I reckon we
> > should enable it for all devices because it will let us remove the deferred
> > error logic we have in AVDA.
> > > 
> > > Actually, do you know where we can find ANR rates to check how this is
going
> > on JB? I assume that's how deadlocks would manifest. Other kinds of failures
> > should result in crash reports at least.
> > 
> > Probably you can use crash/blame with the file path to this point and see
what
> > pops up. I can poke around tomorrow.
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
> > File media/blink/webmediaplayer_impl.cc (right):
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/blink/webmediapl...
> > media/blink/webmediaplayer_impl.cc:1473: void
> > WebMediaPlayerImpl::OnSurfaceRequested(
> > On 2016/11/03 at 20:26:34, watk wrote:
> > > Naming suggestion for when the time comes. This feels more like
> > "SetSurfaceChangeObserver" or something like that to me now. i.e., the
decoder
> > is an observer of surface changes rather than a requester of surfaces.
> > > 
> > > This changed a while back when we started requesting the surface on
fullscreen
> > transitions instead of in response to this call.
> > 
> > Agreed I don't like the name, but since the mechanism is double duty
depending
> > on platform support it's not always an observer. Will see if I can come up
with
> > a better name.
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
> > File media/filters/gpu_video_decoder.cc (right):
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/filters/gpu_vide...
> > media/filters/gpu_video_decoder.cc:344: return;
> > On 2016/11/03 at 20:26:34, watk wrote:
> > > Not sure how this is hit?
> > 
> > Heh, without this we can't change surfaces :) -- that always happens
post-init.
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
> > File media/gpu/android_video_decode_accelerator.cc (right):
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/android_vide...
> > media/gpu/android_video_decode_accelerator.cc:1356: if (pending_surface_id_)
{
> > On 2016/11/03 at 20:51:04, liberato wrote:
> > > if it comes in later, then we might already be using surface texture.  in
that
> > case, SetSurface should probably notice that we're already using surface
texture
> > and not set |pending_surface_id_|.  otherwise, DequeueOutput will wait for
> > outstanding buffers.
> > 
> > This case is already handled in SetSurface and here?
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
> > File media/gpu/avda_codec_image.cc (right):
> > 
> >
https://codereview.chromium.org/2461073002/diff/100001/media/gpu/avda_codec_i...
> > media/gpu/avda_codec_image.cc:55: // GLint bound_service_id = 0;
> > On 2016/11/03 at 20:51:04, liberato wrote:
> > > now that we're swapping out |shared_state_| when setting up a picture for
a
> > new frame, how does it get into this situation?
> > 
> > Just left over testing. Removed.
> 
> do the gl errors show up immediately, or only after a full screen => embedded
transition?

Only after the transition.

[DaleCurtis, 2016-11-07 22:44:05]

Hmm, if I disable the suppressor I get a different error (not the crash I was
expecting):

ERROR:gles2_cmd_decoder_autogen.h(1806)] [.DisplayCompositor-0x9e295400]GL ERROR
:GL_INVALID_ENUM : GetShaderiv: <- error from previous GL command

[DaleCurtis, 2016-11-07 23:03:05]

Looks like the original error is coming from here:

https://www.opengl.org/sdk/docs/man/docbook4/xhtml/glActiveTexture.xml

[liberato (no reviews please), 2016-11-07 23:10:35]

On 2016/11/07 23:03:05, DaleCurtis wrote:
> Looks like the original error is coming from here:
> 
> https://www.opengl.org/sdk/docs/man/docbook4/xhtml/glActiveTexture.xml

seems unlikely that the texture target would change, though.

if you log the beginning/end of AVDAPictureBufferManager::Initialize, and
UseCodecBufferForPicture, do you see that the error occurs inside or outside
those functions?  similarly for CodecImage::UpdateSurfaceTexture.

[DaleCurtis, 2016-11-07 23:30:17]

On 2016/11/07 at 23:10:35, liberato wrote:
> On 2016/11/07 23:03:05, DaleCurtis wrote:
> > Looks like the original error is coming from here:
> > 
> > https://www.opengl.org/sdk/docs/man/docbook4/xhtml/glActiveTexture.xml
> 
> seems unlikely that the texture target would change, though.
> 
> if you log the beginning/end of AVDAPictureBufferManager::Initialize, and
UseCodecBufferForPicture, do you see that the error occurs inside or outside
those functions?  similarly for CodecImage::UpdateSurfaceTexture.

Yeah, it looks like the error has already occurred by the time
PrepareTexturesForRender() is called. No errors are thrown anywhere inside those
functions. I checked that originally, but retested just now and still nothing :|

[DaleCurtis, 2016-11-08 01:00:46]

Description was changed from

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

This dramatically improves the fullscreen transition; it's much
faster and does not cause audio jank.

BUG=662251
TEST=enter/exit fullscreen on M+ device.
==========

to

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

This dramatically improves the fullscreen transition; it's much
faster and does not cause audio jank.

BUG=662251
TEST=enter/exit fullscreen on M+ device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2016-11-08 01:01:41]

Ah, found the issue thanks to watk@. GVD handles clearing textures on the first
picture ready for us... but this didn't happen for the new texture. The fix is
to always clear the texture ourselves and add a DCHECK to prevent folk from
shooting themselves in the foot in the future:

https://codereview.chromium.org/2481943003

[DaleCurtis, 2016-11-08 21:36:17]

Patchset #9 (id:160001) has been deleted

[DaleCurtis, 2016-11-08 21:47:17]

Patchset #9 (id:180001) has been deleted

[DaleCurtis, 2016-11-08 21:48:52]

dalecurtis@chromium.org changed reviewers:
+ dcheng@chromium.org

[DaleCurtis, 2016-11-08 21:48:53]

PTAL. Reworked WMPI logic to be clearer. +dcheng for media/gpu/ipc/common/*
review.

[watk, 2016-11-08 22:59:04]

https://codereview.chromium.org/2461073002/diff/200001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461073002/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:630: }
Seems like this should return an error? If we fail to set the surface we
shouldn't try to continue. Or if we're only trying to catch programming mistakes
we probably shouldn't catch the exceptions?

https://codereview.chromium.org/2461073002/diff/200001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1539: OnSurfaceCreated(overlay_surface_id_);
Much clearer!

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:336:
base::ResetAndReturn(&init_cb_).Run(false);
Need to check if init_cb_.is_null()

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:344: return;
Any chance you could rework this a bit to make it clearer? Not a big deal since
we'll probably refactor this once we get DialogSurface. But for now it's
confusing that CompleteInitialization() can be called after we're initialized
only to set the surface.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:666:
DCHECK_GE(base::android::BuildInfo::GetInstance()->sdk_int(), 23);
Feels like this DCHECK belongs in SetSurface(). i.e., DCHECK(!initialized ||
sdk_int >= 23)

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1266: // Surface changes never
take affect immediately, they will be handled during
s/affect/effect/

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1343: // leave full screen. If we
don't clear the surface immediately during this
s/full screen/fullscreen for consistency

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1353: // The leaving full screen
notification may come in before this point.
s/full screen/fullscreen

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_codec_i...
File media/gpu/avda_codec_image.h (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_codec_i...
media/gpu/avda_codec_image.h:77: // Decoded buffer index that has the image for
us to display.
comment out of date

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:177: DCHECK(it != codec_images_.end());
nice

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:302: bool
AVDAPictureBufferManager::ArePicturesOutstanding() const {
IMO the term "unrendered" communicates the purpose clearly in the name, whereas
"outstanding" doesn't. AreAnyPicturesUnrendered? HasUnrenderedPictures?

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.h:91: enum GLenum { kTextureTarget =
GL_TEXTURE_EXTERNAL_OES };
constexpr instead of enum? Move to the top? If you leave it as an enum, can you
remove the name, or am I missing something? Confuses me to see another GLenum.

https://codereview.chromium.org/2461073002/diff/200001/media/video/video_deco...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/2461073002/diff/200001/media/video/video_deco...
media/video/video_decode_accelerator.h:289: // previosuly set surface in favor
of an internally generated texture.
*previously

[DaleCurtis, 2016-11-08 23:50:11]

https://codereview.chromium.org/2461073002/diff/200001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461073002/diff/200001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:630: }
On 2016/11/08 at 22:59:03, watk wrote:
> Seems like this should return an error? If we fail to set the surface we
shouldn't try to continue. Or if we're only trying to catch programming mistakes
we probably shouldn't catch the exceptions?

Done.

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:344: return;
On 2016/11/08 at 22:59:03, watk wrote:
> Any chance you could rework this a bit to make it clearer? Not a big deal
since we'll probably refactor this once we get DialogSurface. But for now it's
confusing that CompleteInitialization() can be called after we're initialized
only to set the surface.

Hmm, without redesigning how we provide the surface I don't think there's much
cleanup I can do here. Did you have something specific in mind?

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:666:
DCHECK_GE(base::android::BuildInfo::GetInstance()->sdk_int(), 23);
On 2016/11/08 at 22:59:03, watk wrote:
> Feels like this DCHECK belongs in SetSurface(). i.e., DCHECK(!initialized ||
sdk_int >= 23)

Moved to the codec bridge.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1266: // Surface changes never
take affect immediately, they will be handled during
On 2016/11/08 at 22:59:03, watk wrote:
> s/affect/effect/

Done.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1343: // leave full screen. If we
don't clear the surface immediately during this
On 2016/11/08 at 22:59:03, watk wrote:
> s/full screen/fullscreen for consistency

Done.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1353: // The leaving full screen
notification may come in before this point.
On 2016/11/08 at 22:59:03, watk wrote:
> s/full screen/fullscreen

Done.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_codec_i...
File media/gpu/avda_codec_image.h (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_codec_i...
media/gpu/avda_codec_image.h:77: // Decoded buffer index that has the image for
us to display.
On 2016/11/08 at 22:59:03, watk wrote:
> comment out of date

Done.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:302: bool
AVDAPictureBufferManager::ArePicturesOutstanding() const {
On 2016/11/08 at 22:59:03, watk wrote:
> IMO the term "unrendered" communicates the purpose clearly in the name,
whereas "outstanding" doesn't. AreAnyPicturesUnrendered? HasUnrenderedPictures?

Good idea, HasUnrenderedPictures it is.

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2461073002/diff/200001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.h:91: enum GLenum { kTextureTarget =
GL_TEXTURE_EXTERNAL_OES };
On 2016/11/08 at 22:59:03, watk wrote:
> constexpr instead of enum? Move to the top? If you leave it as an enum, can
you remove the name, or am I missing something? Confuses me to see another
GLenum.

Whoops, that was supposed to be the type. Dropped. Still an enum though since
constexpr can't be used without declaring the it as static.

https://codereview.chromium.org/2461073002/diff/200001/media/video/video_deco...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/2461073002/diff/200001/media/video/video_deco...
media/video/video_decode_accelerator.h:289: // previosuly set surface in favor
of an internally generated texture.
On 2016/11/08 at 22:59:04, watk wrote:
> *previously

Done.

[watk, 2016-11-08 23:58:12]

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:344: return;
On 2016/11/08 23:50:10, DaleCurtis wrote:
> On 2016/11/08 at 22:59:03, watk wrote:
> > Any chance you could rework this a bit to make it clearer? Not a big deal
> since we'll probably refactor this once we get DialogSurface. But for now it's
> confusing that CompleteInitialization() can be called after we're initialized
> only to set the surface.
> 
> Hmm, without redesigning how we provide the surface I don't think there's much
> cleanup I can do here. Did you have something specific in mind?

Fair enough. I guess I was thinking we could have a GVD::SetSurface that we bind
to the surface request. And in that: vda->SetSurface(); if (!init_cb_.is_null())
CompleteIniitalization();

[DaleCurtis, 2016-11-09 00:19:03]

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/200001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:344: return;
On 2016/11/08 at 23:58:12, watk wrote:
> On 2016/11/08 23:50:10, DaleCurtis wrote:
> > On 2016/11/08 at 22:59:03, watk wrote:
> > > Any chance you could rework this a bit to make it clearer? Not a big deal
> > since we'll probably refactor this once we get DialogSurface. But for now
it's
> > confusing that CompleteInitialization() can be called after we're
initialized
> > only to set the surface.
> > 
> > Hmm, without redesigning how we provide the surface I don't think there's
much
> > cleanup I can do here. Did you have something specific in mind?
> 
> Fair enough. I guess I was thinking we could have a GVD::SetSurface that we
bind to the surface request. And in that: vda->SetSurface(); if
(!init_cb_.is_null()) CompleteIniitalization();

Ah, this is good, but required some reworking of |cdm_id| to get done.

[watk, 2016-11-09 00:56:36]

lgtm

https://codereview.chromium.org/2461073002/diff/240001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/240001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:335:
base::ResetAndReturn(&init_cb_).Run(false);
Need to check init_cb_ is not null before running it?

[DaleCurtis, 2016-11-09 01:12:15]

https://codereview.chromium.org/2461073002/diff/240001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/2461073002/diff/240001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:335:
base::ResetAndReturn(&init_cb_).Run(false);
On 2016/11/09 at 00:56:36, watk wrote:
> Need to check init_cb_ is not null before running it?

Good catch, done.

[watk, 2016-11-09 01:20:48]

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:117: gfx::Size
AVDAPictureBufferManager::GetPictureBufferSize() const {
Just realized the only caller of this is AVDA and it's implemented by calling
AVDA::GetSize() :)

[dcheng, 2016-11-09 18:16:29]

ipc lgtm

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_codec_i...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_codec_i...
media/gpu/avda_codec_image.cc:144: const scoped_refptr<AVDASharedState>&
shared_state) {
Nit: it's preferred to pass by value and std::move() where appropriate to avoid
refcount thrashing.

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.h:91: enum { kTextureTarget =
GL_TEXTURE_EXTERNAL_OES };
I wonder... can we just use constexpr now?

[liberato (no reviews please), 2016-11-09 18:25:28]

lgtm.  nice!

[DaleCurtis, 2016-11-09 21:58:18]

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_codec_i...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_codec_i...
media/gpu/avda_codec_image.cc:144: const scoped_refptr<AVDASharedState>&
shared_state) {
On 2016/11/09 at 18:16:29, dcheng wrote:
> Nit: it's preferred to pass by value and std::move() where appropriate to
avoid refcount thrashing.

Done.

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.cc (right):

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.cc:117: gfx::Size
AVDAPictureBufferManager::GetPictureBufferSize() const {
On 2016/11/09 at 01:20:48, watk wrote:
> Just realized the only caller of this is AVDA and it's implemented by calling
AVDA::GetSize() :)

Good catch, removed, fixed up code to use DCHECK in AVDA.

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
File media/gpu/avda_picture_buffer_manager.h (right):

https://codereview.chromium.org/2461073002/diff/260001/media/gpu/avda_picture...
media/gpu/avda_picture_buffer_manager.h:91: enum { kTextureTarget =
GL_TEXTURE_EXTERNAL_OES };
On 2016/11/09 at 18:16:29, dcheng wrote:
> I wonder... can we just use constexpr now?

It needs to be declared static when used in a header file (compile error
otherwise), but otherwise works. Style guide seems to say declaring a constexpr
static this way is okay, so done.

[DaleCurtis, 2016-11-09 23:09:11]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-11-09 23:09:11]

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

[commit-bot: I haz the power, 2016-11-09 23:09:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 00:52:50]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2016-11-10 00:57:17]

Description was changed from

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

This dramatically improves the fullscreen transition; it's much
faster and does not cause audio jank.

BUG=662251
TEST=enter/exit fullscreen on M+ device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use MediaCodec.setOutputSurface() for fullscreen transitions on M.

Turns out this isn't so hard to do that we need to wait for the
completion of DialogSurface and AVDAv2.

This dramatically improves the fullscreen transition; it's much
faster and does not cause audio jank.

BUG=662251
TEST=enter/exit fullscreen on M+ device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/4b632fce277d847aa2c5a533478da715086c0b55
Cr-Commit-Position: refs/heads/master@{#431114}
==========

[commit-bot: I haz the power, 2016-11-10 00:57:18]

Patchset 13 (id:??) landed as
https://crrev.com/4b632fce277d847aa2c5a533478da715086c0b55
Cr-Commit-Position: refs/heads/master@{#431114}