Chromecast: Add a way to notify video plane about resolution changes

Chromecast: Add a way to notify video plane about resolution changes

Added OnScreenResolutionChanged to be notified about screen
resolution changes (needed to fix b/18205424), plus a bit of a refactoring-  got rid of platform-specific videoplane registries (duplicated code anyway), introduced a VideoPlaneFactory.

BUG=none

Committed: https://crrev.com/ab5bcf3cdcfe34635e4fa7ea38d2a92e6ec1eedf
Cr-Commit-Position: refs/heads/master@{#308945}

[servolk, 2014-12-17 22:34:01]

servolk@chromium.org changed reviewers:
+ damienv@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[servolk, 2014-12-17 22:34:11]

PTAL

[gunsch, 2014-12-17 22:45:51]

Please try to make the first commit message line more indicative of this CL's
contents.

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

https://codereview.chromium.org/809173002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.cc:52: // Only one video plane is
supported at the moment
if you're defining this entire class inline, why not just have GetVideoPlane()
take no args?

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

https://codereview.chromium.org/809173002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.h:49: // VideoPlane should reposition
itself according to the new screen resolution.
can a plane be repositioned from a screen size, or only resized?

[servolk, 2014-12-18 00:17:37]

I think the CL description is accurate, am I missing something?

Re. repositioning - it's up to the video plane implementations to decide how
exactly they want to handle screen resolution changes. At the moment chromecast
internal implementations will try to preserve video plane position relative to
gfx plane, but that may mean repositioning in the absolute screen coordinates.

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

https://codereview.chromium.org/809173002/diff/1/chromecast/media/cma/backend...
chromecast/media/cma/backend/video_plane.cc:52: // Only one video plane is
supported at the moment
On 2014/12/17 22:45:51, gunsch wrote:
> if you're defining this entire class inline, why not just have GetVideoPlane()
> take no args?

Done.

[servolk, 2014-12-18 01:47:01]

On 2014/12/18 01:23:35, servolk wrote:

ping

[gunsch, 2014-12-18 02:04:51]

To clarify my comment on the CL commit naming (generally I've gotten pushback
from Chromium committers upstream to write upstream commit messages more
carefully, particularly the first line):

* We don't mention "upstreaming from Chromecast", especially as an explanation
for what a CL is/does. The upstream git log is essentially the future permanent
record of Chromecast's code, so mentioning "upstreaming" or "porting" or
anything about our internal repo at this moment is actually counterproductive.

* The first line shows up places like
https://chromium.googlesource.com/chromium/src/+log/master/chromecast, in the
CQ, in bot emails etc., so maybe a title like "Adds a mechanism to notify video
planes of screen resolution changes" better describes the new features of this
CL when looking at-a-glance at our history.

* (Aside:) A personal preference of mine is to start each commit message with
"Chromecast:" or "[Chromecast]", to indicate to people looking at the CQ or
otherwise scanning lists of Chromium-wide CLs instantly know the context. Some
other teams do this, e.g. devtools mark all of their CLs this way.

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
File chromecast/media/cma/backend/video_plane_fake.h (right):

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane_fake.h:17: virtual gfx::Size
GetScreenResolution() override;
not correct, remove "virtual" at TOT

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane_fake.h:23: friend class
VideoPlaneFactory;
hmm, is this better than just having VideoPlaneFake ctor be public? (not sure if
I have a strong opinion, except that "friend classes" should be somewhat
exceptional)

[servolk, 2014-12-18 02:12:25]

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
File chromecast/media/cma/backend/video_plane_fake.h (right):

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane_fake.h:17: virtual gfx::Size
GetScreenResolution() override;
On 2014/12/18 02:04:51, gunsch wrote:
> not correct, remove "virtual" at TOT

Done.

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane_fake.h:23: friend class
VideoPlaneFactory;
On 2014/12/18 02:04:51, gunsch wrote:
> hmm, is this better than just having VideoPlaneFake ctor be public? (not sure
if
> I have a strong opinion, except that "friend classes" should be somewhat
> exceptional)

Done.

[gunsch, 2014-12-18 02:14:21]

lgtm

[lcwu1, 2014-12-18 02:17:31]

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

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:34: // AVSettings instead.
AVSettings is not in the public code, so mentioning it here seems strange. Maybe
we should be a bit more descriptive here and say something like "... from the
info provided by the display hardware such as HDMI/EDID." Also a TODO usually
requires a corresponding bug.

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:48: // plugged in into a new HDMI
port and a new HDMI EDID is received).
nit: "plugged in into" is tautology. :-) Should be either "plugged into" or
"plugged in to".

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:59: static scoped_ptr<VideoPlane>
CreatePlatformVideoPlane();
Why not simply make this a static Create function in VideoPlane?

[servolk, 2014-12-18 02:33:33]

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

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:34: // AVSettings instead.
On 2014/12/18 02:17:31, lcwu1 wrote:
> AVSettings is not in the public code, so mentioning it here seems strange.
Maybe
> we should be a bit more descriptive here and say something like "... from the
> info provided by the display hardware such as HDMI/EDID." Also a TODO usually
> requires a corresponding bug.

Ok, I've removed the TODO completely for now.

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:48: // plugged in into a new HDMI
port and a new HDMI EDID is received).
On 2014/12/18 02:17:31, lcwu1 wrote:
> nit: "plugged in into" is tautology. :-) Should be either "plugged into" or
> "plugged in to".

Done.

https://codereview.chromium.org/809173002/diff/40001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:59: static scoped_ptr<VideoPlane>
CreatePlatformVideoPlane();
On 2014/12/18 02:17:31, lcwu1 wrote:
> Why not simply make this a static Create function in VideoPlane?

Done.

[lcwu1, 2014-12-18 02:51:17]

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

https://codereview.chromium.org/809173002/diff/80001/chromecast/media/cma/bac...
chromecast/media/cma/backend/video_plane.h:55: scoped_ptr<VideoPlane>
CreateVideoPlane();
What I mean is to make this a static function inside VideoPlane class, and this
way, you don't need to have a separate video_plane_factory source file.

[servolk, 2014-12-18 03:01:51]

On 2014/12/18 02:51:17, lcwu1 wrote:
>
https://codereview.chromium.org/809173002/diff/80001/chromecast/media/cma/bac...
> File chromecast/media/cma/backend/video_plane.h (right):
> 
>
https://codereview.chromium.org/809173002/diff/80001/chromecast/media/cma/bac...
> chromecast/media/cma/backend/video_plane.h:55: scoped_ptr<VideoPlane>
> CreateVideoPlane();
> What I mean is to make this a static function inside VideoPlane class, and
this
> way, you don't need to have a separate video_plane_factory source file.

Actually we do need the separate factory source file at the moment. In media.gyp
we include the fake factory when compiling for chromium and use internal CMA
backend factory when branding=Chrome

[lcwu1, 2014-12-18 03:06:50]

lgtm

[servolk, 2014-12-18 03:07:12]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2014-12-18 03:07:41]

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

[commit-bot: I haz the power, 2014-12-18 04:01:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-18 04:02:23]

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