Send enter / exit fullscreen signal to AVDA

Send enter / exit fullscreen signal to AVDA

This CL causes WMPI to notify AVDA when the player element enters /
exits fullscreen.  This will help AVDA to choose whether to use an
overlay or SurfaceTexture to back the video frames.

It also replaces the (surface id, routing token) pair that is sent
via the Provide/RequestOverlayInfoCB with an OverlayInfo struct.
This structure contains the surface id and routing token, plus the
current fullscreen state, if it is known.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2892083002
Cr-Commit-Position: refs/heads/master@{#478048}
Committed: https://chromium.googlesource.com/chromium/src/+/fe8f969833ed376460af94787e2572755e598575

[liberato (no reviews please), 2017-05-18 21:05:27]

Description was changed from

==========
added static constructors


added OverlayInfo

BUG=
==========

to

==========
added static constructors


added OverlayInfo

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[liberato (no reviews please), 2017-05-22 21:40:25]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:40:57]

Dry run: CQ is trying da patch.

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

[liberato (no reviews please), 2017-05-22 21:56:19]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:56:30]

Dry run: 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, 2017-05-23 02:14:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 02:14:20]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-24 18:30:39]

Description was changed from

==========
added static constructors


added OverlayInfo

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Send enter / exit fullscreen signal to AVDA
0123456789012345678901234567890123456789012345678901234567890123456789

This CL causes WMPI to notify AVDA when the player element enters /
exits fullscreen.  This will help AVDA to choose whether to use an
overlay or SurfaceTexture to back the video frames.

It also replaces the (surface id, routing token) pair that is sent
via the Provide/RequestOverlayInfoCB with an OverlayInfo struct.
This structure contains the surface id and routing token, plus the
current fullscreen state, if it is known.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[liberato (no reviews please), 2017-05-24 18:31:16]

Description was changed from

==========
Send enter / exit fullscreen signal to AVDA
0123456789012345678901234567890123456789012345678901234567890123456789

This CL causes WMPI to notify AVDA when the player element enters /
exits fullscreen.  This will help AVDA to choose whether to use an
overlay or SurfaceTexture to back the video frames.

It also replaces the (surface id, routing token) pair that is sent
via the Provide/RequestOverlayInfoCB with an OverlayInfo struct.
This structure contains the surface id and routing token, plus the
current fullscreen state, if it is known.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Send enter / exit fullscreen signal to AVDA

This CL causes WMPI to notify AVDA when the player element enters /
exits fullscreen.  This will help AVDA to choose whether to use an
overlay or SurfaceTexture to back the video frames.

It also replaces the (surface id, routing token) pair that is sent
via the Provide/RequestOverlayInfoCB with an OverlayInfo struct.
This structure contains the surface id and routing token, plus the
current fullscreen state, if it is known.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[liberato (no reviews please), 2017-05-24 18:31:16]

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

[liberato (no reviews please), 2017-05-24 18:34:53]

i'm not sure that i like how OverlayInfo is structured, but i wasn't able to
come up with a better way.  sending all the state all the time was very tricky. 
i also tried including a message field there, with message types like "enter
fullscreen".  however, that made it hard to represent the initial overlay state.

as it is, we can send in all of this info at once as part of the initial config.
 we can also update things as they change.

i'll probably add another unit test or two, especially for
OverlayInfo::operator|=().

none of this actually causes AVDA to take any different action than it does now.

thanks
-fl

[tguilbert, 2017-05-24 19:14:54]

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_inf...
File media/base/overlay_info.cc (right):

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_inf...
media/base/overlay_info.cc:30: if (other.routing_token)
Is there a reason why |is_fullscreen| is not copied?

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_info.h
File media/base/overlay_info.h (right):

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_inf...
media/base/overlay_info.h:27: // that |other| doesn't specify.
NIT: I am not sure that I like this shorthand operator, because the | operator
is normally commutative, but that is not the case here. A more explicit
'CopyInto' or 'CopyFrom' would make it clearer.

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

https://codereview.chromium.org/2892083002/diff/120001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:367:
DCHECK(!config_.overlay_info.surface_id ||
NIT: How do you feel about adding syntactic sugar for
"overlay_info.HasValidSurfaceId". Entirely up to you whether or not you think
it's worth it, or overkill.

https://codereview.chromium.org/2892083002/diff/120001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:437:
*config_.overlay_info.routing_token && overlay_factory_cb_) {
NIT: Same/Similar comment as above for "overlay_info.HasValidRoutingToken".

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

https://codereview.chromium.org/2892083002/diff/120001/media/video/video_deco...
media/video/video_decode_accelerator.h:156: #if 0
Intentional?

[liberato (no reviews please), 2017-05-24 21:21:07]

thanks for the feedback.

still thinking about an appropriate name for |= .

-fl

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_inf...
File media/base/overlay_info.cc (right):

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_inf...
media/base/overlay_info.cc:30: if (other.routing_token)
On 2017/05/24 19:14:54, tguilbert wrote:
> Is there a reason why |is_fullscreen| is not copied?

because it's error-prone.  :)  good catch.

i really don't like this pattern.  too bad that one can't "operator|=() =
default" with base::Optional supporting |= .

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_info.h
File media/base/overlay_info.h (right):

https://codereview.chromium.org/2892083002/diff/120001/media/base/overlay_inf...
media/base/overlay_info.h:27: // that |other| doesn't specify.
On 2017/05/24 19:14:54, tguilbert wrote:
> NIT: I am not sure that I like this shorthand operator, because the | operator
> is normally commutative, but that is not the case here. A more explicit
> 'CopyInto' or 'CopyFrom' would make it clearer.

| is commutative, but this is |= .

still, it's a fair point.  i'll try some other non-operator names.  originally i
tried "CopyInto", but that made it unclear that it was actually modifying rather
than replacing the destination.  I tried "Union", but then i thought that one
often uses '|' for that.

i'll think some more on it.

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

https://codereview.chromium.org/2892083002/diff/120001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:367:
DCHECK(!config_.overlay_info.surface_id ||
On 2017/05/24 19:14:54, tguilbert wrote:
> NIT: How do you feel about adding syntactic sugar for
> "overlay_info.HasValidSurfaceId". Entirely up to you whether or not you think
> it's worth it, or overkill.

i think that it's a good idea.  Done.

https://codereview.chromium.org/2892083002/diff/120001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:437:
*config_.overlay_info.routing_token && overlay_factory_cb_) {
On 2017/05/24 19:14:54, tguilbert wrote:
> NIT: Same/Similar comment as above for "overlay_info.HasValidRoutingToken".

Done.

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

https://codereview.chromium.org/2892083002/diff/120001/media/video/video_deco...
media/video/video_decode_accelerator.h:156: #if 0
On 2017/05/24 19:14:54, tguilbert wrote:
> Intentional?

nope, wanted to be sure that i didn't forget to update the commment.  i did
forget to update the comment.

[liberato (no reviews please), 2017-05-24 22:04:48]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 22:05:37]

Dry run: CQ is trying da patch.

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

[liberato (no reviews please), 2017-05-24 22:06:29]

|= => MergeWith.  also, unittests for MergeWith.

particularly happy with the helper function in the unittest.  didn't realize
that one could do that with member variables.  thought it was for member fns
only.

thanks
-fl

[liberato (no reviews please), 2017-05-24 22:17:31]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 22:19:04]

Dry run: CQ is trying da patch.

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

[liberato (no reviews please), 2017-05-24 22:24:26]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 22:25:08]

Dry run: CQ is trying da patch.

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

[tguilbert, 2017-05-24 22:47:58]

LGTM!

Patch #10 has the best description I've read so far.

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_info.h
File media/base/overlay_info.h (right):

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_inf...
media/base/overlay_info.h:13: #include "media/base/surface_manager.h"
Remove surface_manager.h?

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_inf...
media/base/overlay_info.h:21: static OverlayInfo SurfaceChange(int surface_id);
I assume these will be used somewhere at some point.

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_inf...
media/base/overlay_info.h:44: // SurfaceManager:;kNoSurfaceID to indicate that
no surface should be used.
NIT: Typo. s/:;/::

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

https://codereview.chromium.org/2892083002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1284: if (routing_token &&
overlay_factory_cb_) {
Drive-by NIT: remove brackets for single line if/elif.

[commit-bot: I haz the power, 2017-05-25 02:09:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 02:09:22]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[liberato (no reviews please), 2017-05-25 05:35:19]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 05:35:40]

Dry run: CQ is trying da patch.

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

[liberato (no reviews please), 2017-05-25 05:42:50]

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_info.h
File media/base/overlay_info.h (right):

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_inf...
media/base/overlay_info.h:13: #include "media/base/surface_manager.h"
On 2017/05/24 22:47:58, tguilbert wrote:
> Remove surface_manager.h?

Done.

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_inf...
media/base/overlay_info.h:21: static OverlayInfo SurfaceChange(int surface_id);
On 2017/05/24 22:47:58, tguilbert wrote:
> I assume these will be used somewhere at some point.

wmpi was going to use them, but i guess it doesn't now.  i should have removed
them.  Done.

https://codereview.chromium.org/2892083002/diff/200001/media/base/overlay_inf...
media/base/overlay_info.h:44: // SurfaceManager:;kNoSurfaceID to indicate that
no surface should be used.
On 2017/05/24 22:47:58, tguilbert wrote:
> NIT: Typo. s/:;/::

Done.

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

https://codereview.chromium.org/2892083002/diff/200001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1284: if (routing_token &&
overlay_factory_cb_) {
On 2017/05/24 22:47:58, tguilbert wrote:
> Drive-by NIT: remove brackets for single line if/elif.

Done.

[commit-bot: I haz the power, 2017-05-25 08:03:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 08:03:12]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-25 16:30:47]

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

[liberato (no reviews please), 2017-05-25 16:31:34]

+dcheng: PTAL @ various ipc bits.

thanks
-fl

[watk, 2017-05-25 18:09:37]

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_info.h
File media/base/overlay_info.h (right):

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_inf...
media/base/overlay_info.h:26: using RoutingToken =
base::Optional<base::UnguessableToken>;
Move to the top.

From the style guide:

Within each section, generally prefer grouping similar kinds of declarations
together, and generally prefer the following order: types (including typedef,
using, and nested structs and classes), constants, factory functions,
constructors, assignment operators, destructor, all other methods, data members.

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_inf...
media/base/overlay_info.h:55: bool HasValidSurfaceId() const;
Move above member variables

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_inf...
media/base/overlay_info.h:60: // or an AndroidOverlay routing token.
Second sentence out of date (mostly)

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

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:255:
OverlayInfo::RoutingToken(base::UnguessableToken())) {
Shouldn't this be a nullopt RoutingToken? I don't understand what it means for:
overlay_routing_token_ && *overlay_routing_token_ &&
*overlay_routing_token_->is_empty().

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:418: // overlay mode all the time.
Can you move this comment back to the if statement

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:270: void MaybeSendOverlayInfoToDecoder(bool
send_factory_info);
"factory info" is a pretty opaque term to me. It would be difficult to
understand what causing the decoder to "change factories" means if you weren't
familiar with your recent changes.

I suggest using the term "overlay id" to mean either a surface id or overlay
token.

Is it much work to make the decoder handle getting all the info every time?
Because it's pretty weird how we only set some of the fields in certain cases.

Seems like AVDA just has to do:

if (new_info.HasSameOverlayIdAs(current_info_)) {
  current_info = new_info;
  return;
}

// Do normal creation of factory.

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:787: // use overlays).
This is really confusing. There's three levels of nullability. I think you need
some bools or a wrapper type. I don't have a specific suggestion, but I can tell
you that I spent five minutes trying to make sure I understood this.

I also don't think you need to explain what it means to have a null
RoutingToken. We should treat RoutingToken as an opaque type here. It shouldn't
matter to WMPI that a RoutingToken is actually an optional.

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:790: // Is the player fullscreen?
Comment can be deleted  IMO

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

https://codereview.chromium.org/2892083002/diff/220001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1261: // It's possible that we'll
receive a SetSurface before initializing the
"SetSurface"

https://codereview.chromium.org/2892083002/diff/220001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1264: // surface.  In this case,
just pretend that |surface_id| is the initial one.
"surface_id"

https://codereview.chromium.org/2892083002/diff/220001/media/video/video_deco...
File media/video/video_decode_accelerator.cc (right):

https://codereview.chromium.org/2892083002/diff/220001/media/video/video_deco...
media/video/video_decode_accelerator.cc:49: NOTREACHED() << "Surfaces are not
supported.";
Overlays

[liberato (no reviews please), 2017-05-25 21:26:56]

this one doesn't have base::Optional<> in OverlayInfo.  overall, i like it
better.  WMPI still needs base::Optional<> for the surface id / token, since it
has to represent "requested from RFH / SurfaceManager but not yet received".

there's some additional complexity in AVDA::SetOverlayInfo to detect duplicates.
 it doesn't seem too bad.

WDYT?

thanks
-fl

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_info.h
File media/base/overlay_info.h (right):

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_inf...
media/base/overlay_info.h:26: using RoutingToken =
base::Optional<base::UnguessableToken>;
On 2017/05/25 18:09:35, watk wrote:
> Move to the top.
> 
> From the style guide:
> 
> Within each section, generally prefer grouping similar kinds of declarations
> together, and generally prefer the following order: types (including typedef,
> using, and nested structs and classes), constants, factory functions,
> constructors, assignment operators, destructor, all other methods, data
members.
> 

Done.

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_inf...
media/base/overlay_info.h:55: bool HasValidSurfaceId() const;
On 2017/05/25 18:09:35, watk wrote:
> Move above member variables

Done.

https://codereview.chromium.org/2892083002/diff/220001/media/base/overlay_inf...
media/base/overlay_info.h:60: // or an AndroidOverlay routing token.
On 2017/05/25 18:09:35, watk wrote:
> Second sentence out of date (mostly)

Done.

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

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:255:
OverlayInfo::RoutingToken(base::UnguessableToken())) {
On 2017/05/25 18:09:36, watk wrote:
> Shouldn't this be a nullopt RoutingToken? I don't understand what it means
for:
> overlay_routing_token_ && *overlay_routing_token_ &&
> *overlay_routing_token_->is_empty().

it should be ort_(RoutingToken()), which means "we are not allowed to use
overlays".

compare to left unintialized, which would mean "we have asked the callback for a
routing token, but it hasn't gotten back to us yet".

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:418: // overlay mode all the time.
On 2017/05/25 18:09:36, watk wrote:
> Can you move this comment back to the if statement

Done.

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:270: void MaybeSendOverlayInfoToDecoder(bool
send_factory_info);
On 2017/05/25 18:09:36, watk wrote:
> "factory info" is a pretty opaque term to me. It would be difficult to
> understand what causing the decoder to "change factories" means if you weren't
> familiar with your recent changes.
> 
> I suggest using the term "overlay id" to mean either a surface id or overlay
> token.
> 
> Is it much work to make the decoder handle getting all the info every time?
> Because it's pretty weird how we only set some of the fields in certain cases.
> 
> Seems like AVDA just has to do:
> 
> if (new_info.HasSameOverlayIdAs(current_info_)) {
>   current_info = new_info;
>   return;
> }
> 
> // Do normal creation of factory.
> 

Done.

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:787: // use overlays).
On 2017/05/25 18:09:36, watk wrote:
> This is really confusing. There's three levels of nullability. I think you
need
> some bools or a wrapper type. I don't have a specific suggestion, but I can
tell
> you that I spent five minutes trying to make sure I understood this.
> 
> I also don't think you need to explain what it means to have a null
> RoutingToken. We should treat RoutingToken as an opaque type here. It
shouldn't
> matter to WMPI that a RoutingToken is actually an optional.

i cleaned up the description.

i don't think that having explicit bools is going to help; that's exactly the
same as base::Optional<>.  i don't see how it would be any more or less
complicated.

https://codereview.chromium.org/2892083002/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:790: // Is the player fullscreen?
On 2017/05/25 18:09:36, watk wrote:
> Comment can be deleted  IMO

Done.

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

https://codereview.chromium.org/2892083002/diff/220001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1261: // It's possible that we'll
receive a SetSurface before initializing the
On 2017/05/25 18:09:36, watk wrote:
> "SetSurface"

Done.

https://codereview.chromium.org/2892083002/diff/220001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1264: // surface.  In this case,
just pretend that |surface_id| is the initial one.
On 2017/05/25 18:09:36, watk wrote:
> "surface_id"

done, reworked the comment a bit.

https://codereview.chromium.org/2892083002/diff/220001/media/video/video_deco...
File media/video/video_decode_accelerator.cc (right):

https://codereview.chromium.org/2892083002/diff/220001/media/video/video_deco...
media/video/video_decode_accelerator.cc:49: NOTREACHED() << "Surfaces are not
supported.";
On 2017/05/25 18:09:36, watk wrote:
> Overlays

Done.

[watk, 2017-05-25 21:36:21]

lgtm

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

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:393: // it will display properly.
Move comment down to condition it's referring to

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:783: // |overlay_routing_token_| won't have a
value.
Nice, thanks

[dcheng, 2017-06-02 15:30:06]

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:784: base::Optional<OverlayInfo::RoutingToken>
overlay_routing_token_;
Hmm, isn't this an Optional<Optional<OverlayInfo::RoutingToken>> due to the
typedefs? Can this be just one level of Optional?

[liberato (no reviews please), 2017-06-02 18:18:59]

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

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:393: // it will display properly.
On 2017/05/25 21:36:20, watk wrote:
> Move comment down to condition it's referring to

Done.

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:784: base::Optional<OverlayInfo::RoutingToken>
overlay_routing_token_;
On 2017/06/02 15:30:06, dcheng wrote:
> Hmm, isn't this an Optional<Optional<OverlayInfo::RoutingToken>> due to the
> typedefs? Can this be just one level of Optional?

it is O<O<>>, but i don't see how it can be reduced easily.

the problem is that we need a way to denote explicitly "no token". however,
there's no distiguished value for UnguessableToken; an is_null() token isn't
serializable.

the inner Optional<> is used to denote this -- a !has_value() RoutingToken means
"no token" here and in other places like AVDA.

the outer Optional<> is used by wmpi to figure out if it knows the answer to the
question 'what is my routing token', like it does for Optional<int> surface_id
elsewhere.  a !has_value |overlay_routing_token_| means that the question has
been asked via callback, but not yet answered.

i could make an explicit bool out of the outer one, but that's pretty much what
Optional<> does.  keeping it together seemed clearer.

suggestions welcome.

[dcheng, 2017-06-05 18:32:10]

LGTM

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:784: base::Optional<OverlayInfo::RoutingToken>
overlay_routing_token_;
On 2017/06/02 18:18:59, liberato wrote:
> On 2017/06/02 15:30:06, dcheng wrote:
> > Hmm, isn't this an Optional<Optional<OverlayInfo::RoutingToken>> due to the
> > typedefs? Can this be just one level of Optional?
> 
> it is O<O<>>, but i don't see how it can be reduced easily.
> 
> the problem is that we need a way to denote explicitly "no token". however,
> there's no distiguished value for UnguessableToken; an is_null() token isn't
> serializable.
> 
> the inner Optional<> is used to denote this -- a !has_value() RoutingToken
means
> "no token" here and in other places like AVDA.
> 
> the outer Optional<> is used by wmpi to figure out if it knows the answer to
the
> question 'what is my routing token', like it does for Optional<int> surface_id
> elsewhere.  a !has_value |overlay_routing_token_| means that the question has
> been asked via callback, but not yet answered.
> 
> i could make an explicit bool out of the outer one, but that's pretty much
what
> Optional<> does.  keeping it together seemed clearer.
> 
> suggestions welcome.

Perhaps just integrate your reply here into the comment; I found it quite
instructive.

[dcheng, 2017-06-05 18:35:56]

On 2017/06/05 18:32:10, dcheng wrote:
> LGTM
> 
>
https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.h (right):
> 
>
https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.h:784:
base::Optional<OverlayInfo::RoutingToken>
> overlay_routing_token_;
> On 2017/06/02 18:18:59, liberato wrote:
> > On 2017/06/02 15:30:06, dcheng wrote:
> > > Hmm, isn't this an Optional<Optional<OverlayInfo::RoutingToken>> due to
the
> > > typedefs? Can this be just one level of Optional?
> > 
> > it is O<O<>>, but i don't see how it can be reduced easily.
> > 
> > the problem is that we need a way to denote explicitly "no token". however,
> > there's no distiguished value for UnguessableToken; an is_null() token isn't
> > serializable.
> > 
> > the inner Optional<> is used to denote this -- a !has_value() RoutingToken
> means
> > "no token" here and in other places like AVDA.
> > 
> > the outer Optional<> is used by wmpi to figure out if it knows the answer to
> the
> > question 'what is my routing token', like it does for Optional<int>
surface_id
> > elsewhere.  a !has_value |overlay_routing_token_| means that the question
has
> > been asked via callback, but not yet answered.
> > 
> > i could make an explicit bool out of the outer one, but that's pretty much
> what
> > Optional<> does.  keeping it together seemed clearer.
> > 
> > suggestions welcome.
> 
> Perhaps just integrate your reply here into the comment; I found it quite
> instructive.

(Or just use a separate bool, it'll be clearer that way and probably not require
the comment. That's what I would personally recommend)

[liberato (no reviews please), 2017-06-07 20:06:34]

thanks for the comments!

-fl

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2892083002/diff/300001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.h:784: base::Optional<OverlayInfo::RoutingToken>
overlay_routing_token_;
On 2017/06/05 18:32:10, dcheng wrote:
> On 2017/06/02 18:18:59, liberato wrote:
> > On 2017/06/02 15:30:06, dcheng wrote:
> > > Hmm, isn't this an Optional<Optional<OverlayInfo::RoutingToken>> due to
the
> > > typedefs? Can this be just one level of Optional?
> > 
> > it is O<O<>>, but i don't see how it can be reduced easily.
> > 
> > the problem is that we need a way to denote explicitly "no token". however,
> > there's no distiguished value for UnguessableToken; an is_null() token isn't
> > serializable.
> > 
> > the inner Optional<> is used to denote this -- a !has_value() RoutingToken
> means
> > "no token" here and in other places like AVDA.
> > 
> > the outer Optional<> is used by wmpi to figure out if it knows the answer to
> the
> > question 'what is my routing token', like it does for Optional<int>
surface_id
> > elsewhere.  a !has_value |overlay_routing_token_| means that the question
has
> > been asked via callback, but not yet answered.
> > 
> > i could make an explicit bool out of the outer one, but that's pretty much
> what
> > Optional<> does.  keeping it together seemed clearer.
> > 
> > suggestions welcome.
> 
> Perhaps just integrate your reply here into the comment; I found it quite
> instructive.

i ended up taking your other suggestion, to split up the Optional<> into a
RoutingToken and a bool.

[liberato (no reviews please), 2017-06-07 20:06:41]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-06-07 20:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org, watk@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2892083002/#ps340001
(title: "removed Optional<> in wmpi for routing token")

[commit-bot: I haz the power, 2017-06-07 20:07:36]

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, 2017-06-07 20:10:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 20:10:18]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[liberato (no reviews please), 2017-06-07 20:51:31]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 20:56:55]

Dry run: 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, 2017-06-07 20:57:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 20:57:29]

Dry run: CQ cannot get SignCLA result. Please try later.

[liberato (no reviews please), 2017-06-07 21:15:43]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-06-07 21:15:44]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
tguilbert@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2892083002/#ps360001
(title: "rebased.")

[commit-bot: I haz the power, 2017-06-07 21:16:13]

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, 2017-06-07 21:31:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 21:31:51]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[liberato (no reviews please), 2017-06-08 17:16:24]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-06-08 17:16:51]

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, 2017-06-08 19:17:39]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1496942184517060,
"parent_rev": "0ad840645e519cfc4ebb9af29ae479e29d296401", "commit_rev":
"fe8f969833ed376460af94787e2572755e598575"}

[commit-bot: I haz the power, 2017-06-08 19:17:53]

Description was changed from

==========
Send enter / exit fullscreen signal to AVDA

This CL causes WMPI to notify AVDA when the player element enters /
exits fullscreen.  This will help AVDA to choose whether to use an
overlay or SurfaceTexture to back the video frames.

It also replaces the (surface id, routing token) pair that is sent
via the Provide/RequestOverlayInfoCB with an OverlayInfo struct.
This structure contains the surface id and routing token, plus the
current fullscreen state, if it is known.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Send enter / exit fullscreen signal to AVDA

This CL causes WMPI to notify AVDA when the player element enters /
exits fullscreen.  This will help AVDA to choose whether to use an
overlay or SurfaceTexture to back the video frames.

It also replaces the (surface id, routing token) pair that is sent
via the Provide/RequestOverlayInfoCB with an OverlayInfo struct.
This structure contains the surface id and routing token, plus the
current fullscreen state, if it is known.

BUG=662995
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2892083002
Cr-Commit-Position: refs/heads/master@{#478048}
Committed:
https://chromium.googlesource.com/chromium/src/+/fe8f969833ed376460af94787e25...
==========

[commit-bot: I haz the power, 2017-06-08 19:17:55]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/fe8f969833ed376460af94787e25...