Inform MediaObserver when video natural size changes.

Inform MediaObserver when video natural size changes.

BUG=678339
Review-Url: https://codereview.chromium.org/2620433002
Cr-Commit-Position: refs/heads/master@{#442107}
Committed: https://chromium.googlesource.com/chromium/src/+/516ef6d9c0c19352fb80ecaa4e635e60501125fb

[xjz, 2017-01-06 01:14:03]

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

[commit-bot: I haz the power, 2017-01-06 01:14:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xjz, 2017-01-06 01:56:52]

Description was changed from

==========
Inform MediaObserver when video natural size changes.

BUG=678339
==========

to

==========
Inform MediaObserver when video natural size changes.

BUG=678339
==========

[xjz, 2017-01-06 01:56:52]

xjz@chromium.org changed reviewers:
+ sandersd@chromium.org

[commit-bot: I haz the power, 2017-01-06 02:29:23]

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

[commit-bot: I haz the power, 2017-01-06 02:29:23]

Dry run: This issue passed the CQ dry run.

[xjz, 2017-01-06 02:29:38]

sandersd@: As discussed in crbug/678339, this CL informs MediaObserver when
OnVideoNaturalSizeChanged() is called. PTAL. Thanks!

[sandersd (OOO until July 31), 2017-01-06 19:29:18]

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1321: pipeline_metadata_.natural_size =
rotated_size;
Looks like there is a pre-existing bug here, CreateWatchTimeReporter() shouldn't
be called until after |pipeline_metadata_.natural_size| is updated.

Please fix.

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1326: metadata.natural_size = size;
Almost certainly you want the rotated_size. Since that's what's in
|pipeline_metadata_|, just pass that?

[xjz, 2017-01-06 21:59:15]

sandersd: Thanks for reviewing. Addressed your comments. PTAL again.

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1321: pipeline_metadata_.natural_size =
rotated_size;
On 2017/01/06 19:29:18, sandersd wrote:
> Looks like there is a pre-existing bug here, CreateWatchTimeReporter()
shouldn't
> be called until after |pipeline_metadata_.natural_size| is updated.
> 
> Please fix.

Done.

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1326: metadata.natural_size = size;
On 2017/01/06 19:29:18, sandersd wrote:
> Almost certainly you want the rotated_size. Since that's what's in
> |pipeline_metadata_|, just pass that?

The size will be rotated in RemotingRendererController if needed
(https://cs.chromium.org/chromium/src/media/remoting/remoting_renderer_control...).
Therefore pass the original size here. Actually it is confusing to have some
PipelineMetadata store orginal natural size, and some store the rotated size.
Since now all application requires roted size, how about let it always store the
rotated size? That is, we only need to rotate it once in PipelineImpl when this
metadata is created. If this sgty, I can do the refactoring in a separate CL.

[sandersd (OOO until July 31), 2017-01-06 22:18:07]

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1326: metadata.natural_size = size;
On 2017/01/06 21:59:15, xjz wrote:
> On 2017/01/06 19:29:18, sandersd wrote:
> > Almost certainly you want the rotated_size. Since that's what's in
> > |pipeline_metadata_|, just pass that?
> 
> The size will be rotated in RemotingRendererController if needed
>
(https://cs.chromium.org/chromium/src/media/remoting/remoting_renderer_control...).
> Therefore pass the original size here. Actually it is confusing to have some
> PipelineMetadata store orginal natural size, and some store the rotated size.
> Since now all application requires roted size, how about let it always store
the
> rotated size? That is, we only need to rotate it once in PipelineImpl when
this
> metadata is created. If this sgty, I can do the refactoring in a separate CL.

OnMetadata() also applies the rotation, so if RemotingRendererController is also
applying a rotation it's probably wrong.

Moving this up the stack in a followup SGTM.

[xjz, 2017-01-06 22:23:57]

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1326: metadata.natural_size = size;
On 2017/01/06 22:18:07, sandersd wrote:
> On 2017/01/06 21:59:15, xjz wrote:
> > On 2017/01/06 19:29:18, sandersd wrote:
> > > Almost certainly you want the rotated_size. Since that's what's in
> > > |pipeline_metadata_|, just pass that?
> > 
> > The size will be rotated in RemotingRendererController if needed
> >
>
(https://cs.chromium.org/chromium/src/media/remoting/remoting_renderer_control...).
> > Therefore pass the original size here. Actually it is confusing to have some
> > PipelineMetadata store orginal natural size, and some store the rotated
size.
> > Since now all application requires roted size, how about let it always store
> the
> > rotated size? That is, we only need to rotate it once in PipelineImpl when
> this
> > metadata is created. If this sgty, I can do the refactoring in a separate
CL.
> 
> OnMetadata() also applies the rotation, so if RemotingRendererController is
also
> applying a rotation it's probably wrong.
> 
> Moving this up the stack in a followup SGTM.

OnMetadata() also passed the original size (not the rotated one) to
MediaObserver
(https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=1...).
I will do a follow up refactoring CL.

[sandersd (OOO until July 31), 2017-01-06 22:36:34]

On 2017/01/06 22:23:57, xjz wrote:
>
https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2620433002/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:1326: metadata.natural_size = size;
> On 2017/01/06 22:18:07, sandersd wrote:
> > On 2017/01/06 21:59:15, xjz wrote:
> > > On 2017/01/06 19:29:18, sandersd wrote:
> > > > Almost certainly you want the rotated_size. Since that's what's in
> > > > |pipeline_metadata_|, just pass that?
> > > 
> > > The size will be rotated in RemotingRendererController if needed
> > >
> >
>
(https://cs.chromium.org/chromium/src/media/remoting/remoting_renderer_control...).
> > > Therefore pass the original size here. Actually it is confusing to have
some
> > > PipelineMetadata store orginal natural size, and some store the rotated
> size.
> > > Since now all application requires roted size, how about let it always
store
> > the
> > > rotated size? That is, we only need to rotate it once in PipelineImpl when
> > this
> > > metadata is created. If this sgty, I can do the refactoring in a separate
> CL.
> > 
> > OnMetadata() also applies the rotation, so if RemotingRendererController is
> also
> > applying a rotation it's probably wrong.
> > 
> > Moving this up the stack in a followup SGTM.
> 
> OnMetadata() also passed the original size (not the rotated one) to
> MediaObserver
>
(https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=1...).
> I will do a follow up refactoring CL.

Ah I see, thanks.

lgtm.

[xjz, 2017-01-06 22:37:14]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2017-01-06 22:37:46]

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-01-07 00:23:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483742234203260,
"parent_rev": "df4f7e43c1e79a85246f8297f1f0111a967e6a56", "commit_rev":
"516ef6d9c0c19352fb80ecaa4e635e60501125fb"}

[commit-bot: I haz the power, 2017-01-07 00:23:38]

Description was changed from

==========
Inform MediaObserver when video natural size changes.

BUG=678339
==========

to

==========
Inform MediaObserver when video natural size changes.

BUG=678339

Review-Url: https://codereview.chromium.org/2620433002
Cr-Commit-Position: refs/heads/master@{#442107}
Committed:
https://chromium.googlesource.com/chromium/src/+/516ef6d9c0c19352fb80ecaa4e63...
==========

[commit-bot: I haz the power, 2017-01-07 00:23:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/516ef6d9c0c19352fb80ecaa4e63...