Fixes to Cast use of overlays for video frames

Fixes to Cast use of overlays for video frames

A few miscellaneous fixes found during testing:
* Post VideoPlane::SetGeometry calls to media thread (found that some
implementations require this).
* Finalize CastMediaShlib on media thread to ensure it's not accessed
by media code after it's destroyed.  I think this was a pre-existing
bug although it's hard to trigger.
* Avoid unnecessary calls to VideoPlane::SetGeometry when arguments
haven't changed.
* Use sync point to ensure video frame dummy texture gets created before
it's used.

BUG=469374
Committed: https://crrev.com/22a67f27595a92312d34c88835ec5d521f9f7e70
Cr-Commit-Position: refs/heads/master@{#337872}

[halliwell, 2015-07-06 19:02:59]

halliwell@chromium.org changed reviewers:
+ erickung@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[gunsch, 2015-07-06 19:06:27]

https://codereview.chromium.org/1223793009/diff/1/ui/ozone/platform/cast/DEPS
File ui/ozone/platform/cast/DEPS (right):

https://codereview.chromium.org/1223793009/diff/1/ui/ozone/platform/cast/DEPS...
ui/ozone/platform/cast/DEPS:2: "+chromecast/browser/media/cma_message_loop.h",
Oooof. No.

Is there a way we can pass a TaskRunner reference into the Ozone platform code
at initialization time?

[halliwell, 2015-07-06 19:28:47]

On 2015/07/06 19:06:27, gunsch wrote:
> https://codereview.chromium.org/1223793009/diff/1/ui/ozone/platform/cast/DEPS
> File ui/ozone/platform/cast/DEPS (right):
> 
>
https://codereview.chromium.org/1223793009/diff/1/ui/ozone/platform/cast/DEPS...
> ui/ozone/platform/cast/DEPS:2: "+chromecast/browser/media/cma_message_loop.h",
> Oooof. No.
> 
> Is there a way we can pass a TaskRunner reference into the Ozone platform code
> at initialization time?

Not that I know of ... the Ozone stuff is constructed by calling
CreateOzonePlatformCast.

One possibility I was thinking of is to just leave this as a straight
SetGeometry call, and have the vendor implementation of VideoPlane be
responsible for posting that to the cma thread.  But ...
* it seems a little awkward to force all vendors to implement this
* we'd have to pass in a task runner to the vendor code after creating the cma
thread ourselves, and avoid using chromium types, which has some awkwardness. 
Or, will the cma -> shared lib changes end up with vendors managing the cma
thread themselves?

[halliwell, 2015-07-08 15:59:33]

On 2015/07/06 19:28:47, halliwell wrote:
> On 2015/07/06 19:06:27, gunsch wrote:
> >
https://codereview.chromium.org/1223793009/diff/1/ui/ozone/platform/cast/DEPS
> > File ui/ozone/platform/cast/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1223793009/diff/1/ui/ozone/platform/cast/DEPS...
> > ui/ozone/platform/cast/DEPS:2:
"+chromecast/browser/media/cma_message_loop.h",
> > Oooof. No.
> > 
> > Is there a way we can pass a TaskRunner reference into the Ozone platform
code
> > at initialization time?
> 
> Not that I know of ... the Ozone stuff is constructed by calling
> CreateOzonePlatformCast.
> 
> One possibility I was thinking of is to just leave this as a straight
> SetGeometry call, and have the vendor implementation of VideoPlane be
> responsible for posting that to the cma thread.  But ...
> * it seems a little awkward to force all vendors to implement this
> * we'd have to pass in a task runner to the vendor code after creating the cma
> thread ourselves, and avoid using chromium types, which has some awkwardness. 
> Or, will the cma -> shared lib changes end up with vendors managing the cma
> thread themselves?

Ok, patch set 2 is based on CmaMessageLoop rename/move
(https://codereview.chromium.org/1224923003/).

PTAL.

[gunsch, 2015-07-08 16:02:52]

lgtm % couple questions

https://codereview.chromium.org/1223793009/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1223793009/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:328:
media::CastMediaShlib::Initialize(cmd_line->argv());
do we also want to post this to the media thread?

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/overlay_manager_cast.cc (right):

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/overlay_manager_cast.cc:68: void
CheckOverlaySupport(OverlaySurfaceCandidateList* surfaces) override {
readability nit: there are now four consecutive close braces below and I wasn't
able to easily scan what goes where. could you (optionally, either/both):

1) not inline this definition into the header

2) change "if (candidate.plane_z_order == -1) { ... }" to "if
(candidate.plane_z_order != -1) { continue; }" and drop a level of indentation?

[erickung1, 2015-07-08 17:12:33]

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/overlay_manager_cast.cc (right):

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/overlay_manager_cast.cc:55: bool ExactlyEqual(const
chromecast::RectF& r1, const chromecast::RectF& r2) {
wonder if this can become util function?

Although graphics_types.h is in the public folder now and we intend not to have
.cc function implemented.

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/overlay_manager_cast.cc:68: void
CheckOverlaySupport(OverlaySurfaceCandidateList* surfaces) override {
On 2015/07/08 16:02:52, gunsch wrote:
> readability nit: there are now four consecutive close braces below and I
wasn't
> able to easily scan what goes where. could you (optionally, either/both):
> 
> 1) not inline this definition into the header
> 
> 2) change "if (candidate.plane_z_order == -1) { ... }" to "if
> (candidate.plane_z_order != -1) { continue; }" and drop a level of
indentation?

or 

for (...) {
  if (candidate.plane_z_order != -1)
    continue;
  
  /* block of code */

  if (candidate.transform == transform_ &&
      ExactlyEqual(display_rect, display_rect_))
    continue;

  /* block of code: assign value and post task */
  return;

}

  
}

[halliwell, 2015-07-08 17:16:41]

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/overlay_manager_cast.cc (right):

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/overlay_manager_cast.cc:55: bool ExactlyEqual(const
chromecast::RectF& r1, const chromecast::RectF& r2) {
On 2015/07/08 17:12:33, erickung1 wrote:
> wonder if this can become util function?
> 
> Although graphics_types.h is in the public folder now and we intend not to
have
> .cc function implemented.

I'm not sure how widely useful exact float comparisons will be for this struct
... I'd suggest moving it if we end up needing again (presumably has to be an
inline function in a public/ header)

https://codereview.chromium.org/1223793009/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/overlay_manager_cast.cc:68: void
CheckOverlaySupport(OverlaySurfaceCandidateList* surfaces) override {
On 2015/07/08 17:12:33, erickung1 wrote:
> On 2015/07/08 16:02:52, gunsch wrote:
> > readability nit: there are now four consecutive close braces below and I
> wasn't
> > able to easily scan what goes where. could you (optionally, either/both):
> > 
> > 1) not inline this definition into the header
> > 
> > 2) change "if (candidate.plane_z_order == -1) { ... }" to "if
> > (candidate.plane_z_order != -1) { continue; }" and drop a level of
> indentation?
> 
> or 
> 
> for (...) {
>   if (candidate.plane_z_order != -1)
>     continue;
>   
>   /* block of code */
> 
>   if (candidate.transform == transform_ &&
>       ExactlyEqual(display_rect, display_rect_))
>     continue;
> 
>   /* block of code: assign value and post task */
>   return;
> 
> }
> 
>   
> }

Moved out of class definition and used continue as well.

[halliwell, 2015-07-08 17:17:41]

https://codereview.chromium.org/1223793009/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1223793009/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:328:
media::CastMediaShlib::Initialize(cmd_line->argv());
On 2015/07/08 16:02:52, gunsch wrote:
> do we also want to post this to the media thread?

Yes, done - I think it would be great to be able to say to vendors that all of
the libcast_media calls are made on the same thread.

[gunsch, 2015-07-08 17:20:30]

lgtm

https://codereview.chromium.org/1223793009/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1223793009/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:328:
chromecast::media::MediaMessageLoop::GetTaskRunner()->PostTask(
nit: don't need "chromecast" namespace (here and below)

[halliwell, 2015-07-08 17:25:10]

https://codereview.chromium.org/1223793009/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1223793009/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:328:
chromecast::media::MediaMessageLoop::GetTaskRunner()->PostTask(
On 2015/07/08 17:20:30, gunsch wrote:
> nit: don't need "chromecast" namespace (here and below)

Done.

[erickung1, 2015-07-08 17:38:39]

lgtm

https://codereview.chromium.org/1223793009/diff/60001/chromecast/media/cma/fi...
File chromecast/media/cma/filters/hole_frame_factory.cc (right):

https://codereview.chromium.org/1223793009/diff/60001/chromecast/media/cma/fi...
chromecast/media/cma/filters/hole_frame_factory.cc:23: image_id_(0),
|sync_point_| should be set to 0 here for case if |use_legacy_hole_punching_| is
true

[halliwell, 2015-07-08 17:47:17]

https://codereview.chromium.org/1223793009/diff/60001/chromecast/media/cma/fi...
File chromecast/media/cma/filters/hole_frame_factory.cc (right):

https://codereview.chromium.org/1223793009/diff/60001/chromecast/media/cma/fi...
chromecast/media/cma/filters/hole_frame_factory.cc:23: image_id_(0),
On 2015/07/08 17:38:39, erickung1 wrote:
> |sync_point_| should be set to 0 here for case if |use_legacy_hole_punching_|
is
> true

We don't use the sync point in that case, but I updated anyway to initialise it
for clarity and consistency with other fields.

[halliwell, 2015-07-08 17:49:18]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2015-07-08 17:49:19]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
erickung@chromium.org
Link to the patchset: https://codereview.chromium.org/1223793009/#ps80001
(title: "init sync point")

[commit-bot: I haz the power, 2015-07-08 17:50:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1223793009/80001

[commit-bot: I haz the power, 2015-07-08 18:16:56]

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

[commit-bot: I haz the power, 2015-07-08 18:16:57]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[halliwell, 2015-07-08 18:28:19]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2015-07-08 18:28:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
erickung@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1223793009/#ps100001
(title: "Added dependency on media_base")

[commit-bot: I haz the power, 2015-07-08 18:28:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1223793009/100001

[commit-bot: I haz the power, 2015-07-08 18:54:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-08 18:55:36]

Patchset 6 (id:??) landed as
https://crrev.com/22a67f27595a92312d34c88835ec5d521f9f7e70
Cr-Commit-Position: refs/heads/master@{#337872}