Don't require VDAs to return all PictureBuffers at once.

Don't require VDAs to return all PictureBuffers at once.

The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.

This CL allows the VDA to provide a flag with its SupportedProfiles,
collectively called the VDA Capabilities, that indicates that it
cannot promise to return all of the PictureBuffers at once.  In
practice, GpuVideoDecoder uses this flag to always return false from
CanReadWithoutStalling, once picture buffers are assigned.

BUG=531606
Committed: https://crrev.com/57587790264882a5e0b83f90b896a81d7c0b7212
Cr-Commit-Position: refs/heads/master@{#364387}

[liberato (no reviews please), 2015-12-03 21:19:19]

Description was changed from

==========
Don't require VDAs to return all PictureBuffers at once.

The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.

This CL allows the VDA to provide a flag with its SupportedProfiles
that indicates it cannot promise to return all of the PictureBuffers
at once.  In practice, GpuVideoDecoder uses this flag to always
return false from CanReadWithoutStalling.

BUG=531606
==========

to

==========
Don't require VDAs to return all PictureBuffers at once.

The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.

This CL allows the VDA to provide a flag with its SupportedProfiles
that indicates it cannot promise to return all of the PictureBuffers
at once.  In practice, GpuVideoDecoder uses this flag to always
return false from CanReadWithoutStalling.

BUG=531606
==========

[liberato (no reviews please), 2015-12-03 21:19:19]

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

[liberato (no reviews please), 2015-12-03 21:21:01]

here's the !CanReadWithoutStalling implementation that uses SupportedProfiles
instead.  i like it better.

thanks
-fl

https://codereview.chromium.org/1490333005/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1490333005/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:92: strategy_(new
kBackingStrategy()),
interesting aside: this isn't, and wasn't before, protected by the unified media
pipeline.  rtc will use the zero copy strategy if one compiles with
media_use_ffmpeg=1, regardless of the runtime setting.

https://code.google.com/p/chromium/issues/detail?id=565561

[sandersd (OOO until July 31), 2015-12-03 21:44:32]

lgtm

[DaleCurtis, 2015-12-03 21:59:47]

lgtm

https://codereview.chromium.org/1490333005/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1490333005/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:56: #define
kBackingStrategy AndroidDeferredRenderingBackingStrategy
ALL_CAPS

[Pawel Osciak, 2015-12-04 11:09:45]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2015-12-04 11:09:45]

https://codereview.chromium.org/1490333005/diff/1/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1490333005/diff/1/gpu/config/gpu_info.h#newco...
gpu/config/gpu_info.h:62: uint32 flags;
Do we need this in each profile? Do we expect these to differ between different
profiles?

Could we perhaps generalize GetSupportedProfiles IPC to GetDecoderCapabilities
and make it contain a set of flags and the SupportedProfiles vector separately
instead please?

https://codereview.chromium.org/1490333005/diff/1/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/1490333005/diff/1/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:144: VideoDecodeAccelerator::SupportedProfile&
matching_profile_out);
Non-const reference arguments are prohibited by coding style
(https://google.github.io/styleguide/cppguide.html#Reference_Arguments).

https://codereview.chromium.org/1490333005/diff/1/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1490333005/diff/1/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:35: // Flags that can be passed when
providing picture buffers.
Should this be changed?

[liberato (no reviews please), 2015-12-04 18:28:58]

added VDA::GetCapabilities to replace GetSupportedProfiles, and moved the flag
there.

-fl

https://codereview.chromium.org/1490333005/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1490333005/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:56: #define
kBackingStrategy AndroidDeferredRenderingBackingStrategy
On 2015/12/03 21:59:46, DaleCurtis wrote:
> ALL_CAPS

DONE

https://codereview.chromium.org/1490333005/diff/1/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1490333005/diff/1/gpu/config/gpu_info.h#newco...
gpu/config/gpu_info.h:62: uint32 flags;
On 2015/12/04 11:09:45, Pawel Osciak wrote:
> Do we need this in each profile? Do we expect these to differ between
different
> profiles?
> 
> Could we perhaps generalize GetSupportedProfiles IPC to GetDecoderCapabilities
> and make it contain a set of flags and the SupportedProfiles vector separately
> instead please?

Done.

https://codereview.chromium.org/1490333005/diff/1/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/1490333005/diff/1/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.h:144: VideoDecodeAccelerator::SupportedProfile&
matching_profile_out);
On 2015/12/04 11:09:45, Pawel Osciak wrote:
> Non-const reference arguments are prohibited by coding style
> (https://google.github.io/styleguide/cppguide.html#Reference_Arguments).

Done.

https://codereview.chromium.org/1490333005/diff/1/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1490333005/diff/1/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:35: // Flags that can be passed when
providing picture buffers.
On 2015/12/04 11:09:45, Pawel Osciak wrote:
> Should this be changed?

Done.

https://codereview.chromium.org/1490333005/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/1490333005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:41: // Static query for
supported profiles. This query calls the appropriate
will update this comment.

[DaleCurtis, 2015-12-04 18:40:14]

https://codereview.chromium.org/1490333005/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1490333005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:26: enum
{ kNumPictureBuffers = 4 };
Should this be limits:kMaxFrames + 1 like in the copy back strategy?

[liberato (no reviews please), 2015-12-04 18:50:04]

https://codereview.chromium.org/1490333005/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1490333005/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:26: enum
{ kNumPictureBuffers = 4 };
On 2015/12/04 18:40:13, DaleCurtis wrote:
> Should this be limits:kMaxFrames + 1 like in the copy back strategy?

done, and removed GetNumPictureBuffers.

[liberato (no reviews please), 2015-12-04 18:50:56]

Description was changed from

==========
Don't require VDAs to return all PictureBuffers at once.

The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.

This CL allows the VDA to provide a flag with its SupportedProfiles
that indicates it cannot promise to return all of the PictureBuffers
at once.  In practice, GpuVideoDecoder uses this flag to always
return false from CanReadWithoutStalling.

BUG=531606
==========

to

==========
Don't require VDAs to return all PictureBuffers at once.
    
    The android deferred rendering backing strategy cannot always
    guarantee a minimum number of outstanding PictureBuffers beyond
    one at the client.  Failure to do so can cause the pipeline to
    hang waiting for more pictures before returning any.
    
    This CL allows the VDA to provide a flag with its SupportedProfiles,
    collectively called the VDA Capabilities, that indicates that it
    cannot promise to return all of the PictureBuffers at once.  In
    practice, GpuVideoDecoder uses this flag to always return false from
    CanReadWithoutStalling, once picture buffers are assigned.
    
    BUG=531606
==========

[liberato (no reviews please), 2015-12-04 18:51:26]

Description was changed from

==========
Don't require VDAs to return all PictureBuffers at once.
    
    The android deferred rendering backing strategy cannot always
    guarantee a minimum number of outstanding PictureBuffers beyond
    one at the client.  Failure to do so can cause the pipeline to
    hang waiting for more pictures before returning any.
    
    This CL allows the VDA to provide a flag with its SupportedProfiles,
    collectively called the VDA Capabilities, that indicates that it
    cannot promise to return all of the PictureBuffers at once.  In
    practice, GpuVideoDecoder uses this flag to always return false from
    CanReadWithoutStalling, once picture buffers are assigned.
    
    BUG=531606
==========

to

==========
Don't require VDAs to return all PictureBuffers at once.
    
The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.
    
This CL allows the VDA to provide a flag with its SupportedProfiles,
collectively called the VDA Capabilities, that indicates that it
cannot promise to return all of the PictureBuffers at once.  In
practice, GpuVideoDecoder uses this flag to always return false from
CanReadWithoutStalling, once picture buffers are assigned.
    
BUG=531606
==========

[sandersd (OOO until July 31), 2015-12-04 19:10:24]

This just gets better and better \o/.

https://codereview.chromium.org/1490333005/diff/40001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1490333005/diff/40001/media/video/video_decod...
media/video/video_decode_accelerator.h:42: kNoFlags = 0,
And now here we've ended up with two different enum styles in the same file. I
don't even know what I want anymore :-(

(Relatedly: there were at least two chromium-dev threads about bitfield enums,
and they didn't come to any conclusions other than that contexpr is still
banned.)

[Pawel Osciak, 2015-12-05 00:18:55]

https://chromiumcodereview.appspot.com/1490333005/diff/20002/content/common/g...
File content/common/gpu/media/gpu_video_accelerator_util.cc (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/content/common/g...
content/common/gpu/media/gpu_video_accelerator_util.cc:41:
static_cast<media::VideoDecodeAccelerator::Capabilities::Flags>(
Is the cast needed? Both are uint32s...

https://chromiumcodereview.appspot.com/1490333005/diff/20002/content/renderer...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/content/renderer...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:85:
GetVideoDecodeAcceleratorCapabilities() override;
Please indent.

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/filters/gp...
media/filters/gpu_video_decoder.cc:362: (!can_stall_anytime_ &&
available_pictures_ > 0);
The indents here seem wrong...

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/filters/gp...
media/filters/gpu_video_decoder.cc:656: for (const auto& supported_profile :
supported_profiles) {
Nit: Personally I'd just s/supported_profiles/capabilities.supported_profiles/
and remove l.654-655, but up to you.

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/filters/gp...
File media/filters/gpu_video_decoder.h (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/filters/gp...
media/filters/gpu_video_decoder.h:211: // stall as long as one picture buffer is
outstanding.
Please clarify what "outstanding" means, where the buffer is, what is its state
(decoded/empty/etc.), etc.

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/filters/gp...
media/filters/gpu_video_decoder.h:212: bool can_stall_anytime_;
What does "anytime" mean? Should we drop it? It's also confusing, since I
believe if can_stall_anytime_ == true and buffers are not allocated, then it
won't stall actually?

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/renderers/...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/renderers/...
media/renderers/gpu_video_accelerator_factories.h:103:
GetVideoDecodeAcceleratorCapabilities() = 0;
Indent?

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/video/vide...
File media/video/video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/video/vide...
media/video/video_decode_accelerator.cc:33: :
profile(media::VIDEO_CODEC_PROFILE_UNKNOWN) {}
Nit: we should probably either change style for all braces (i.e. at line 35
too), or none, for consistency.

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1490333005/diff/20002/media/video/vide...
media/video/video_decode_accelerator.h:46: CAN_STALL_ANY_TIME = 1 << 0,
My feeling is that "stall" is a very specific term that a person not familiar
with how our media pipeline works will likely not understand. Actually, I'm
still not sure I understand this fully myself. "Any time" also feels quite
confusing to me, what does it really tell us?

I'd really appreciate a precise explanation here if possible please. For example
(and please correct this if this is wrong, I am not sure if I am understanding
"stall" in this context correctly either):

"A stall happens when a VDA does not return some or all decoded and ready to be
displayed pictures via PictureReady() to the client, even though it is expected
to. In that case... [...explanation show should the client behave then...]"

Thanks.

[liberato (no reviews please), 2015-12-07 19:01:34]

liberato@chromium.org changed reviewers:
+ dcheng@chromium.org, kbr@chromium.org

[liberato (no reviews please), 2015-12-07 19:04:40]

dcheng: can you provide owners feedback for gpu_messages?
kbr: can you provide owners feedback for gpu/ and content/gpu?
posciak: i believe that i addressed your feedback except for several formatting
issues.  i don't see how to make 'cl format' not undo those changes.

thanks
-fl

https://codereview.chromium.org/1490333005/diff/40001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1490333005/diff/40001/media/video/video_decod...
media/video/video_decode_accelerator.h:42: kNoFlags = 0,
On 2015/12/04 19:10:24, sandersd wrote:
> And now here we've ended up with two different enum styles in the same file. I
> don't even know what I want anymore :-(
> 
> (Relatedly: there were at least two chromium-dev threads about bitfield enums,
> and they didn't come to any conclusions other than that contexpr is still
> banned.)

true.  changed to NO_FLAGS to be consistent. changing everything else to
kIllegalState is just too much for this CL.

https://codereview.chromium.org/1490333005/diff/20002/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.cc (right):

https://codereview.chromium.org/1490333005/diff/20002/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:41:
static_cast<media::VideoDecodeAccelerator::Capabilities::Flags>(
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> Is the cast needed? Both are uint32s...

yes: error: invalid conversion from 'uint32 {aka unsigned int}' to
'media::VideoDecodeAccelerator::Capabilities::Flags' [-fpermissive]

plus, it makes the intention more explicit.

https://codereview.chromium.org/1490333005/diff/20002/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/1490333005/diff/20002/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:85:
GetVideoDecodeAcceleratorCapabilities() override;
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> Please indent.

unfortunately, 'git cl format' will just un-indent it again.  there is at least
one other instance in this file (GetGLContextLock @80).

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

https://codereview.chromium.org/1490333005/diff/20002/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:362: (!can_stall_anytime_ &&
available_pictures_ > 0);
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> The indents here seem wrong...

not sure, but it's what cl format does.

https://codereview.chromium.org/1490333005/diff/20002/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:656: for (const auto& supported_profile :
supported_profiles) {
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> Nit: Personally I'd just s/supported_profiles/capabilities.supported_profiles/
> and remove l.654-655, but up to you.

Done.

https://codereview.chromium.org/1490333005/diff/20002/media/filters/gpu_video...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/1490333005/diff/20002/media/filters/gpu_video...
media/filters/gpu_video_decoder.h:211: // stall as long as one picture buffer is
outstanding.
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> Please clarify what "outstanding" means, where the buffer is, what is its
state
> (decoded/empty/etc.), etc.

done.  please let me know if the new text is more clear.

https://codereview.chromium.org/1490333005/diff/20002/media/filters/gpu_video...
media/filters/gpu_video_decoder.h:212: bool can_stall_anytime_;
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> What does "anytime" mean? Should we drop it? It's also confusing, since I
> believe if can_stall_anytime_ == true and buffers are not allocated, then it
> won't stall actually?

true, which is why i tried to frame it as "one picture buffer outstanding" in
the docs.  i agree with the sentiment, but couldn't find a shorter name for it. 
suggestions are welcome.

https://codereview.chromium.org/1490333005/diff/20002/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/1490333005/diff/20002/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:103:
GetVideoDecodeAcceleratorCapabilities() = 0;
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> Indent?

cl format will simply put it back.

https://codereview.chromium.org/1490333005/diff/20002/media/video/video_decod...
File media/video/video_decode_accelerator.cc (right):

https://codereview.chromium.org/1490333005/diff/20002/media/video/video_decod...
media/video/video_decode_accelerator.cc:33: :
profile(media::VIDEO_CODEC_PROFILE_UNKNOWN) {}
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> Nit: we should probably either change style for all braces (i.e. at line 35
> too), or none, for consistency.

Done, changed 35 to match.

https://codereview.chromium.org/1490333005/diff/20002/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1490333005/diff/20002/media/video/video_decod...
media/video/video_decode_accelerator.h:46: CAN_STALL_ANY_TIME = 1 << 0,
On 2015/12/05 00:18:55, Pawel Osciak wrote:
> My feeling is that "stall" is a very specific term that a person not familiar
> with how our media pipeline works will likely not understand. Actually, I'm
> still not sure I understand this fully myself. "Any time" also feels quite
> confusing to me, what does it really tell us?
> 
> I'd really appreciate a precise explanation here if possible please. For
example
> (and please correct this if this is wrong, I am not sure if I am understanding
> "stall" in this context correctly either):
> 
> "A stall happens when a VDA does not return some or all decoded and ready to
be
> displayed pictures via PictureReady() to the client, even though it is
expected
> to. In that case... [...explanation show should the client behave then...]"
> 
> Thanks.

i agree that it is rather specific.  i chose the name to match
CanReadWithoutStalling.  the problem is that it isn't exactly the right word
there, either.  it is more "CanReadWithoutMaybeDeadlocking" -- nobody actually
guarantees that the decoded frames are available immediately, only that they
will become available if one sends enough input and waits long enough.

i've rewritten the comment.

[Ken Russell (switch to Gerrit), 2015-12-07 22:06:12]

Mostly rubber-stamp LGTM based on others' reviews. Couple of comments.

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:37: // during
playback.  crbug.com/:531588 .
Please fix URL.

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

https://codereview.chromium.org/1490333005/diff/130001/media/video/video_deco...
media/video/video_decode_accelerator.h:52: // provided via PictureReady.
I don't know this code, but this semantic seems like it'd be valid for the VDA
to allow only one outstanding PictureBuffer. I can't imagine Chrome's rendering
pipeline working well if this were the case since usually there's at least one
pending frame in the pipeline. Would it be beneficial to change the semantic to
say that a certain small number of PictureBuffers are allowed to be outstanding?
Do VDAs typically define this number?

[dcheng, 2015-12-08 04:57:59]

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:232: IPC_STRUCT_TRAITS_MEMBER(flags)
How does this work? I don't see an IPC_ENUM_TRAITS for the new enum, but it
appears to still compile?

[liberato (no reviews please), 2015-12-08 15:55:57]

thanks for the feedback!

-fl

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:232: IPC_STRUCT_TRAITS_MEMBER(flags)
On 2015/12/08 04:57:59, dcheng wrote:
> How does this work? I don't see an IPC_ENUM_TRAITS for the new enum, but it
> appears to still compile?

because it's an int32.  including video_decode_accelerator.h isn't allowed in
gpu_info.h, so it's just an int32 rather than keeping two versions of the enum
in sync.

i suppose that it could be moved to some other, common place, if that's better. 
since the flags are intended to be bitwise-or'd anyway, i figured that the max
value wouldn't be that informative anyway.

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:37: // during
playback.  crbug.com/:531588 .
On 2015/12/07 22:06:11, Ken Russell wrote:
> Please fix URL.

Done.

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

https://codereview.chromium.org/1490333005/diff/130001/media/video/video_deco...
media/video/video_decode_accelerator.h:52: // provided via PictureReady.
On 2015/12/07 22:06:11, Ken Russell wrote:
> I don't know this code, but this semantic seems like it'd be valid for the VDA
> to allow only one outstanding PictureBuffer. I can't imagine Chrome's
rendering
> pipeline working well if this were the case since usually there's at least one
> pending frame in the pipeline. Would it be beneficial to change the semantic
to
> say that a certain small number of PictureBuffers are allowed to be
outstanding?
> Do VDAs typically define this number?

unfortunately, we don't get to choose how many picture buffers can be
outstanding in some cases.  android's MediaCodec doesn't guarantee that more
than one decoded frame will be delivered to the client (us) before we return one
to it.  often, we'll get 3+ decoded frames at once.

this change lets us get ahead when MediaCodec allows it, but not make any
guarantees about it.  in practice, this means that we usually have 3 or 4 frames
in the renderer, but things don't deadlock when we don't.

[Pawel Osciak, 2015-12-09 01:31:49]

https://codereview.chromium.org/1490333005/diff/20002/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/1490333005/diff/20002/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:85:
GetVideoDecodeAcceleratorCapabilities() override;
On 2015/12/07 19:04:39, liberato wrote:
> On 2015/12/05 00:18:55, Pawel Osciak wrote:
> > Please indent.
> 
> unfortunately, 'git cl format' will just un-indent it again.  there is at
least
> one other instance in this file (GetGLContextLock @80).

Hm, strange, GetVideoEncodeAcceleratorSupportedProfiles() below is indented
though and git cl format doesn't change it?

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

https://codereview.chromium.org/1490333005/diff/20002/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:362: (!can_stall_anytime_ &&
available_pictures_ > 0);
On 2015/12/07 19:04:39, liberato wrote:
> On 2015/12/05 00:18:55, Pawel Osciak wrote:
> > The indents here seem wrong...
> 
> not sure, but it's what cl format does.

Maybe it gets confused by the inline comment? I think this warrants a more
detailed comment than the existing one anyway though, so perhaps we could have a
comment explaining the whole condition above the return please?

https://codereview.chromium.org/1490333005/diff/20002/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/1490333005/diff/20002/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:103:
GetVideoDecodeAcceleratorCapabilities() = 0;
On 2015/12/07 19:04:39, liberato wrote:
> On 2015/12/05 00:18:55, Pawel Osciak wrote:
> > Indent?
> 
> cl format will simply put it back.

It's strange it doesn't put back l.107. This may be a bug in git cl format
(either way).

https://codereview.chromium.org/1490333005/diff/150001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/1490333005/diff/150001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.h:211: // to the client.  If true, then the VDA
may refuse to send more picture
Hmm... What would you think about:

"If true, the client cannot expect the VDA to produce any new decoded frames,
until it returns all PictureBuffers it may be holding back to the VDA. In other
words, the VDA may require all PictureBuffers to be able to proceed with
decoding the next frame."

https://codereview.chromium.org/1490333005/diff/150001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.h:214: bool can_stall_anytime_;
My point is, I feel that "anytime" doesn't help explain the issue. "Can stall"
already expresses "anytime" in my opinion.

How about calling it "needs_all_picture_buffers_to_decode_" ?

It is a bit of a mouthful, but expresses the issue precisely and we only use it
in one conditional.

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

https://codereview.chromium.org/1490333005/diff/150001/media/video/video_deco...
media/video/video_decode_accelerator.h:52: // provided via PictureReady.
Nice! Really clear. Thank you.

https://codereview.chromium.org/1490333005/diff/150001/media/video/video_deco...
media/video/video_decode_accelerator.h:53: CAN_STALL_ANY_TIME = 1 << 0,
Might I suggest NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE for the name though (please
see my comment in gpu_video_decoder.h).

https://codereview.chromium.org/1490333005/diff/150001/media/video/video_deco...
media/video/video_decode_accelerator.h:57: Flags flags;
Ah now I see why the cast was needed, somehow missed this. This member shouldn't
be an enum, because enum variables can only have one value, while we intend to
use "flags" as a bit field. This should be an uint32.

[dcheng, 2015-12-09 01:41:50]

https://codereview.chromium.org/1490333005/diff/20002/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/1490333005/diff/20002/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:103:
GetVideoDecodeAcceleratorCapabilities() = 0;
On 2015/12/09 at 01:31:48, Pawel Osciak wrote:
> On 2015/12/07 19:04:39, liberato wrote:
> > On 2015/12/05 00:18:55, Pawel Osciak wrote:
> > > Indent?
> > 
> > cl format will simply put it back.
> 
> It's strange it doesn't put back l.107. This may be a bug in git cl format
(either way).

I don't think this is a bug. This is one of the points that the style guide
doesn't explicitly make a ruling on, so clang-format just picked one way to be
consistent.

You could make a good argument that this is how templates are formatted, e.g.:
template <typename T>
class scoped_ptr {
  // ...
};

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:232: IPC_STRUCT_TRAITS_MEMBER(flags)
On 2015/12/08 at 15:55:57, liberato wrote:
> On 2015/12/08 04:57:59, dcheng wrote:
> > How does this work? I don't see an IPC_ENUM_TRAITS for the new enum, but it
> > appears to still compile?
> 
> because it's an int32.  including video_decode_accelerator.h isn't allowed in
gpu_info.h, so it's just an int32 rather than keeping two versions of the enum
in sync.
> 
> i suppose that it could be moved to some other, common place, if that's
better.  since the flags are intended to be bitwise-or'd anyway, i figured that
the max value wouldn't be that informative anyway.

Yeah, max value doesn't add much here, but I think there's still value to using
the actual enum type.

[liberato (no reviews please), 2015-12-09 22:15:31]

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1490333005/diff/130001/content/common/gpu/gpu...
content/common/gpu/gpu_messages.h:232: IPC_STRUCT_TRAITS_MEMBER(flags)
On 2015/12/09 01:41:50, dcheng wrote:
> On 2015/12/08 at 15:55:57, liberato wrote:
> > On 2015/12/08 04:57:59, dcheng wrote:
> > > How does this work? I don't see an IPC_ENUM_TRAITS for the new enum, but
it
> > > appears to still compile?
> > 
> > because it's an int32.  including video_decode_accelerator.h isn't allowed
in
> gpu_info.h, so it's just an int32 rather than keeping two versions of the enum
> in sync.
> > 
> > i suppose that it could be moved to some other, common place, if that's
> better.  since the flags are intended to be bitwise-or'd anyway, i figured
that
> the max value wouldn't be that informative anyway.
> 
> Yeah, max value doesn't add much here, but I think there's still value to
using
> the actual enum type.

based on posciak@'s comments, the field is now a uint32 in both places.

https://codereview.chromium.org/1490333005/diff/150001/media/filters/gpu_vide...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/1490333005/diff/150001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.h:211: // to the client.  If true, then the VDA
may refuse to send more picture
On 2015/12/09 01:31:49, Pawel Osciak wrote:
> Hmm... What would you think about:
> 
> "If true, the client cannot expect the VDA to produce any new decoded frames,
> until it returns all PictureBuffers it may be holding back to the VDA. In
other
> words, the VDA may require all PictureBuffers to be able to proceed with
> decoding the next frame."

Done.

https://codereview.chromium.org/1490333005/diff/150001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.h:214: bool can_stall_anytime_;
On 2015/12/09 01:31:49, Pawel Osciak wrote:
> My point is, I feel that "anytime" doesn't help explain the issue. "Can stall"
> already expresses "anytime" in my opinion.
> 
> How about calling it "needs_all_picture_buffers_to_decode_" ?
> 
> It is a bit of a mouthful, but expresses the issue precisely and we only use
it
> in one conditional.

Done.

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

https://codereview.chromium.org/1490333005/diff/150001/media/video/video_deco...
media/video/video_decode_accelerator.h:52: // provided via PictureReady.
On 2015/12/09 01:31:49, Pawel Osciak wrote:
> Nice! Really clear. Thank you.

Acknowledged.

https://codereview.chromium.org/1490333005/diff/150001/media/video/video_deco...
media/video/video_decode_accelerator.h:53: CAN_STALL_ANY_TIME = 1 << 0,
On 2015/12/09 01:31:49, Pawel Osciak wrote:
> Might I suggest NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE for the name though
(please
> see my comment in gpu_video_decoder.h).

Done.

https://codereview.chromium.org/1490333005/diff/150001/media/video/video_deco...
media/video/video_decode_accelerator.h:57: Flags flags;
On 2015/12/09 01:31:49, Pawel Osciak wrote:
> Ah now I see why the cast was needed, somehow missed this. This member
shouldn't
> be an enum, because enum variables can only have one value, while we intend to
> use "flags" as a bit field. This should be an uint32.

done, and kept the enum to hold the symbolic names of the bits.

[Pawel Osciak, 2015-12-10 02:03:23]

Thank you very much for your patience with this!

lgtm % question

https://chromiumcodereview.appspot.com/1490333005/diff/170001/media/filters/g...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1490333005/diff/170001/media/filters/g...
media/filters/gpu_video_decoder.cc:366: (!needs_all_picture_buffers_to_decode_
&& available_pictures_ > 0);
I'm wondering, given the clarified definition, should this be:

(next_picture_buffer_id == 0 ||
 (!needs_all_picture_buffers_to_decode_ && available_pictures_ > 0) ||
 available_pictures_ == assigned_picture_buffers.size())
?

[dcheng, 2015-12-10 07:09:48]

lgtm

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

https://codereview.chromium.org/1490333005/diff/170001/media/video/video_deco...
media/video/video_decode_accelerator.h:41: enum Flags {
Is there a convention around sizing the enum explicitly if you're going to use
it as a bitset on an explicitly sized int?

https://codereview.chromium.org/1490333005/diff/170001/media/video/video_deco...
media/video/video_decode_accelerator.h:57: uint32 flags;
Nit: uint32_t (uint32 is going away)

[liberato (no reviews please), 2015-12-10 15:56:07]

thanks for all the feedback!

-fl

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

https://codereview.chromium.org/1490333005/diff/170001/media/filters/gpu_vide...
media/filters/gpu_video_decoder.cc:366: (!needs_all_picture_buffers_to_decode_
&& available_pictures_ > 0);
On 2015/12/10 02:03:23, Pawel Osciak wrote:
> I'm wondering, given the clarified definition, should this be:
> 
> (next_picture_buffer_id == 0 ||
>  (!needs_all_picture_buffers_to_decode_ && available_pictures_ > 0) ||
>  available_pictures_ == assigned_picture_buffers.size())
> ?

good catch, i think so.  will verify.

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

https://codereview.chromium.org/1490333005/diff/170001/media/video/video_deco...
media/video/video_decode_accelerator.h:41: enum Flags {
On 2015/12/10 07:09:48, dcheng wrote:
> Is there a convention around sizing the enum explicitly if you're going to use
> it as a bitset on an explicitly sized int?

i didn't see anything in the style guide about it.  existing code has examples
both ways, didn't see a clear winner.  keeping as is for now.

https://codereview.chromium.org/1490333005/diff/170001/media/video/video_deco...
media/video/video_decode_accelerator.h:57: uint32 flags;
On 2015/12/10 07:09:48, dcheng wrote:
> Nit: uint32_t (uint32 is going away)

Done, and in gpu_info.h

[liberato (no reviews please), 2015-12-10 15:56:29]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-12-10 15:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, sandersd@chromium.org, kbr@chromium.org,
dcheng@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1490333005/#ps190001
(title: "cl feedback.")

[commit-bot: I haz the power, 2015-12-10 15:57:06]

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

[commit-bot: I haz the power, 2015-12-10 17:16:35]

Description was changed from

==========
Don't require VDAs to return all PictureBuffers at once.
    
The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.
    
This CL allows the VDA to provide a flag with its SupportedProfiles,
collectively called the VDA Capabilities, that indicates that it
cannot promise to return all of the PictureBuffers at once.  In
practice, GpuVideoDecoder uses this flag to always return false from
CanReadWithoutStalling, once picture buffers are assigned.
    
BUG=531606
==========

to

==========
Don't require VDAs to return all PictureBuffers at once.
    
The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.
    
This CL allows the VDA to provide a flag with its SupportedProfiles,
collectively called the VDA Capabilities, that indicates that it
cannot promise to return all of the PictureBuffers at once.  In
practice, GpuVideoDecoder uses this flag to always return false from
CanReadWithoutStalling, once picture buffers are assigned.
    
BUG=531606
==========

[commit-bot: I haz the power, 2015-12-10 17:16:35]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-12-10 17:17:45]

Description was changed from

==========
Don't require VDAs to return all PictureBuffers at once.
    
The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.
    
This CL allows the VDA to provide a flag with its SupportedProfiles,
collectively called the VDA Capabilities, that indicates that it
cannot promise to return all of the PictureBuffers at once.  In
practice, GpuVideoDecoder uses this flag to always return false from
CanReadWithoutStalling, once picture buffers are assigned.
    
BUG=531606
==========

to

==========
Don't require VDAs to return all PictureBuffers at once.

The android deferred rendering backing strategy cannot always
guarantee a minimum number of outstanding PictureBuffers beyond
one at the client.  Failure to do so can cause the pipeline to
hang waiting for more pictures before returning any.

This CL allows the VDA to provide a flag with its SupportedProfiles,
collectively called the VDA Capabilities, that indicates that it
cannot promise to return all of the PictureBuffers at once.  In
practice, GpuVideoDecoder uses this flag to always return false from
CanReadWithoutStalling, once picture buffers are assigned.

BUG=531606

Committed: https://crrev.com/57587790264882a5e0b83f90b896a81d7c0b7212
Cr-Commit-Position: refs/heads/master@{#364387}
==========

[commit-bot: I haz the power, 2015-12-10 17:17:47]

Patchset 11 (id:??) landed as
https://crrev.com/57587790264882a5e0b83f90b896a81d7c0b7212
Cr-Commit-Position: refs/heads/master@{#364387}