Chromecast: media interfaces for hardware-backed video plane.

Chromecast: media interfaces for hardware-backed video plane.

R=lcwu@chromium.org,damienv@chromium.org
BUG=408189
Committed: https://crrev.com/5d1cbbb149438debc8c34871f6f2c33beff1062e
Cr-Commit-Position: refs/heads/master@{#308456}

[damienv1, 2014-12-13 01:51:00]

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
File chromecast/media/cma/backend/video_plane.h (right):

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:22: COORDINATE_TYPE_GRAPHICS_PLANE =
0,
nit: blank line

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:24: COORDINATE_TYPE_SCREEN_RESOLUTION
= 1,
Should be renamed to COORDINATE_TYPE_VIDEO_PLANE_RESOLUTION if the intent is to
give coordinates using video plane resolution (which I think it is).

I know that for Eureka, currently, video plane resolution == screen resolution
but it does not have to.

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:27: public:
nit: not needed, already in the public section...

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:32: virtual gfx::Size
GetScreenResolution() = 0;
Either remove this function (if the intent is to get the screen resolution) or
rename it to GetVideoPlaneResolution()

[gunsch, 2014-12-13 02:01:29]

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
File chromecast/media/cma/backend/video_plane.h (right):

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:22: COORDINATE_TYPE_GRAPHICS_PLANE =
0,
On 2014/12/13 01:51:00, damienv1 wrote:
> nit: blank line

Done.

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:24: COORDINATE_TYPE_SCREEN_RESOLUTION
= 1,
On 2014/12/13 01:51:00, damienv1 wrote:
> Should be renamed to COORDINATE_TYPE_VIDEO_PLANE_RESOLUTION if the intent is
to
> give coordinates using video plane resolution (which I think it is).
> 
> I know that for Eureka, currently, video plane resolution == screen resolution
> but it does not have to.

Done.

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:27: public:
On 2014/12/13 01:51:00, damienv1 wrote:
> nit: not needed, already in the public section...

Done.

https://codereview.chromium.org/802833002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:32: virtual gfx::Size
GetScreenResolution() = 0;
On 2014/12/13 01:51:00, damienv1 wrote:
> Either remove this function (if the intent is to get the screen resolution) or
> rename it to GetVideoPlaneResolution()

Done.

[damienv1, 2014-12-15 16:42:40]

lgtm % nit

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
File chromecast/media/cma/backend/video_plane.h (right):

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:24: // Output display screen as
coordinate type.
nit: update comment

[ddorwin, 2014-12-15 21:02:26]

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

[ddorwin, 2014-12-15 21:02:27]

Drive-by comment.

Also, how does this relate to similar concepts in Ozone?

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
File chromecast/media/cma/backend/video_plane.h (right):

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:32: virtual gfx::Size
GetVideoPlaneResolution() = 0;
Return by value can't be forward-declared (line 12).

[gunsch, 2014-12-15 22:20:16]

@ddorwin: hard to say, I'm not too familiar with Ozone. We use this to represent
a hardware-backed video plane, largely for informing the hardware to update its
size when we get an update from the compositor. The CMA code is also
graphics-agnostic, in the sense that the main interfaces shouldn't depend on
Ozone, though an implementation might.

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
File chromecast/media/cma/backend/video_plane.h (right):

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:24: // Output display screen as
coordinate type.
On 2014/12/15 16:42:40, damienv1 wrote:
> nit: update comment

Done.

https://codereview.chromium.org/802833002/diff/20001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:32: virtual gfx::Size
GetVideoPlaneResolution() = 0;
On 2014/12/15 21:02:27, ddorwin wrote:
> Return by value can't be forward-declared (line 12).

Disagreed, per http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

[gunsch, 2014-12-15 22:21:41]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-12-15 22:22:34]

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

[commit-bot: I haz the power, 2014-12-15 23:30:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-15 23:30:48]

Patchset 3 (id:??) landed as
https://crrev.com/5d1cbbb149438debc8c34871f6f2c33beff1062e
Cr-Commit-Position: refs/heads/master@{#308456}