Relocate SurfaceTexture usage from AVDA GLImage into shared state.

Relocate SurfaceTexture usage from AVDA GLImage into shared state.

GetTransforMatrix is only correct for the most recent call to
UpdateTexImage(), which implies it should be shared among all
images instead of specific to each one. As such move it over to
the shared state.

Once done there's no other usage of SurfaceTexture in the image,
so we can also move UpdateTexImage over to the shared state to
avoid comments like "must call UpdateTransformMatrix after calling
UpdateTexImage... etc"

BUG=646603
TEST=video still plays

Committed: https://crrev.com/304de87aa03df6fbbcafeee0983a7e74cf4e56ae
Cr-Commit-Position: refs/heads/master@{#423183}

[DaleCurtis, 2016-09-20 23:31:24]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2016-09-20 23:32:27]

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

[liberato (no reviews please), 2016-09-21 06:05:59]

good idea.

lgtm % nit.

-fl

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
media/gpu/avda_codec_image.cc:225: YInvertMatrix(matrix);
this should probably move into shared state too.  tobiasjs and i came to the
conclusion that we should hide the need for a y flip entirely from chromium.

if you don't go that route, then please update the comment in shared_state.h to
reflect that shared_state::GetTextureMatrix returns a matrix that needs to be y
flipped in order to match the StreamTextureMatrix contract.

[DaleCurtis, 2016-09-23 22:59:57]

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
media/gpu/avda_codec_image.cc:225: YInvertMatrix(matrix);
On 2016/09/21 at 06:05:59, liberato wrote:
> this should probably move into shared state too.  tobiasjs and i came to the
conclusion that we should hide the need for a y flip entirely from chromium.
> 
> if you don't go that route, then please update the comment in shared_state.h
to reflect that shared_state::GetTextureMatrix returns a matrix that needs to be
y flipped in order to match the StreamTextureMatrix contract.

Can't move it there since it's used by multiple callers from
GLStreamTextureImage. As a side note, is this code doing the correct thing for
the initial gl_matrix_ provided a construction time? I.e., should we be
y-flipping the default matrix used before we get one from the SurfaceTexture?

[DaleCurtis, 2016-09-23 23:00:17]

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

[commit-bot: I haz the power, 2016-09-23 23:00:49]

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, 2016-09-24 01:05:07]

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

[commit-bot: I haz the power, 2016-09-24 01:05:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[liberato (no reviews please), 2016-09-27 14:32:26]

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
media/gpu/avda_codec_image.cc:225: YInvertMatrix(matrix);
On 2016/09/23 22:59:56, DaleCurtis wrote:
> On 2016/09/21 at 06:05:59, liberato wrote:
> > this should probably move into shared state too.  tobiasjs and i came to the
> conclusion that we should hide the need for a y flip entirely from chromium.
> > 
> > if you don't go that route, then please update the comment in shared_state.h
> to reflect that shared_state::GetTextureMatrix returns a matrix that needs to
be
> y flipped in order to match the StreamTextureMatrix contract.
> 
> Can't move it there since it's used by multiple callers from
> GLStreamTextureImage. As a side note, is this code doing the correct thing for
> the initial gl_matrix_ provided a construction time? I.e., should we be
> y-flipping the default matrix used before we get one from the SurfaceTexture?

i think that we're miscommunicating.  I mean just to move the YInvertMatrix call
into AVDASharedState::GetTransformMatrix to invert |matrix| before returning it.
 there's nobody that asks for the un-flipped version from shared state.  or,
maybe, just y invert |gl_matrix_| in SharedState::UpdateTexImage.

initial matrix: hrm, good point.  i doubt that we want to y flip it again.  if
one moves the y invert into UpdateTexImage, so that |gl_matrix_| is already
y-flipped, then i think that it all works out.

luckily, i don't think that it matters.  i don't think that there's any path
that actually uses the default anymore.  it's a leftover from when it was
propagated via a side-channel to the renderer, and tended to arrive a few frames
later than the first frame.

[DaleCurtis, 2016-09-27 16:58:18]

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
File media/gpu/avda_codec_image.cc (right):

https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
media/gpu/avda_codec_image.cc:225: YInvertMatrix(matrix);
On 2016/09/27 at 14:32:25, liberato wrote:
> On 2016/09/23 22:59:56, DaleCurtis wrote:
> > On 2016/09/21 at 06:05:59, liberato wrote:
> > > this should probably move into shared state too.  tobiasjs and i came to
the
> > conclusion that we should hide the need for a y flip entirely from chromium.
> > > 
> > > if you don't go that route, then please update the comment in
shared_state.h
> > to reflect that shared_state::GetTextureMatrix returns a matrix that needs
to be
> > y flipped in order to match the StreamTextureMatrix contract.
> > 
> > Can't move it there since it's used by multiple callers from
> > GLStreamTextureImage. As a side note, is this code doing the correct thing
for
> > the initial gl_matrix_ provided a construction time? I.e., should we be
> > y-flipping the default matrix used before we get one from the
SurfaceTexture?
> 
> i think that we're miscommunicating.  I mean just to move the YInvertMatrix
call into AVDASharedState::GetTransformMatrix to invert |matrix| before
returning it.  there's nobody that asks for the un-flipped version from shared
state.  or, maybe, just y invert |gl_matrix_| in SharedState::UpdateTexImage.
> 
> initial matrix: hrm, good point.  i doubt that we want to y flip it again.  if
one moves the y invert into UpdateTexImage, so that |gl_matrix_| is already
y-flipped, then i think that it all works out.
> 
> luckily, i don't think that it matters.  i don't think that there's any path
that actually uses the default anymore.  it's a leftover from when it was
propagated via a side-channel to the renderer, and tended to arrive a few frames
later than the first frame.

YInvertMatrix() is a protected function within GLStreamTextureImage and used in
multiple places. Are you proposing to just copy the code or were you envisioning
moving that function?

[liberato (no reviews please), 2016-09-27 22:50:42]

On 2016/09/27 16:58:18, DaleCurtis wrote:
>
https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
> File media/gpu/avda_codec_image.cc (right):
> 
>
https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
> media/gpu/avda_codec_image.cc:225: YInvertMatrix(matrix);
> On 2016/09/27 at 14:32:25, liberato wrote:
> > On 2016/09/23 22:59:56, DaleCurtis wrote:
> > > On 2016/09/21 at 06:05:59, liberato wrote:
> > > > this should probably move into shared state too.  tobiasjs and i came to
> the
> > > conclusion that we should hide the need for a y flip entirely from
chromium.
> > > > 
> > > > if you don't go that route, then please update the comment in
> shared_state.h
> > > to reflect that shared_state::GetTextureMatrix returns a matrix that needs
> to be
> > > y flipped in order to match the StreamTextureMatrix contract.
> > > 
> > > Can't move it there since it's used by multiple callers from
> > > GLStreamTextureImage. As a side note, is this code doing the correct thing
> for
> > > the initial gl_matrix_ provided a construction time? I.e., should we be
> > > y-flipping the default matrix used before we get one from the
> SurfaceTexture?
> > 
> > i think that we're miscommunicating.  I mean just to move the YInvertMatrix
> call into AVDASharedState::GetTransformMatrix to invert |matrix| before
> returning it.  there's nobody that asks for the un-flipped version from shared
> state.  or, maybe, just y invert |gl_matrix_| in SharedState::UpdateTexImage.
> > 
> > initial matrix: hrm, good point.  i doubt that we want to y flip it again. 
if
> one moves the y invert into UpdateTexImage, so that |gl_matrix_| is already
> y-flipped, then i think that it all works out.
> > 
> > luckily, i don't think that it matters.  i don't think that there's any path
> that actually uses the default anymore.  it's a leftover from when it was
> propagated via a side-channel to the renderer, and tended to arrive a few
frames
> later than the first frame.
> 
> YInvertMatrix() is a protected function within GLStreamTextureImage and used
in
> multiple places. Are you proposing to just copy the code or were you
envisioning
> moving that function?

oh!  i forgot that it was in glstreamtextureimage -- originally, i think that it
was going to be a standalone helper.

hrm, that's annoying, then.  probably just forget the whole thing.  maybe switch
the initial matrix to be non-flipped identity, since i think that it's intended.

[liberato (no reviews please), 2016-09-27 22:50:43]

On 2016/09/27 16:58:18, DaleCurtis wrote:
>
https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
> File media/gpu/avda_codec_image.cc (right):
> 
>
https://codereview.chromium.org/2351293003/diff/20001/media/gpu/avda_codec_im...
> media/gpu/avda_codec_image.cc:225: YInvertMatrix(matrix);
> On 2016/09/27 at 14:32:25, liberato wrote:
> > On 2016/09/23 22:59:56, DaleCurtis wrote:
> > > On 2016/09/21 at 06:05:59, liberato wrote:
> > > > this should probably move into shared state too.  tobiasjs and i came to
> the
> > > conclusion that we should hide the need for a y flip entirely from
chromium.
> > > > 
> > > > if you don't go that route, then please update the comment in
> shared_state.h
> > > to reflect that shared_state::GetTextureMatrix returns a matrix that needs
> to be
> > > y flipped in order to match the StreamTextureMatrix contract.
> > > 
> > > Can't move it there since it's used by multiple callers from
> > > GLStreamTextureImage. As a side note, is this code doing the correct thing
> for
> > > the initial gl_matrix_ provided a construction time? I.e., should we be
> > > y-flipping the default matrix used before we get one from the
> SurfaceTexture?
> > 
> > i think that we're miscommunicating.  I mean just to move the YInvertMatrix
> call into AVDASharedState::GetTransformMatrix to invert |matrix| before
> returning it.  there's nobody that asks for the un-flipped version from shared
> state.  or, maybe, just y invert |gl_matrix_| in SharedState::UpdateTexImage.
> > 
> > initial matrix: hrm, good point.  i doubt that we want to y flip it again. 
if
> one moves the y invert into UpdateTexImage, so that |gl_matrix_| is already
> y-flipped, then i think that it all works out.
> > 
> > luckily, i don't think that it matters.  i don't think that there's any path
> that actually uses the default anymore.  it's a leftover from when it was
> propagated via a side-channel to the renderer, and tended to arrive a few
frames
> later than the first frame.
> 
> YInvertMatrix() is a protected function within GLStreamTextureImage and used
in
> multiple places. Are you proposing to just copy the code or were you
envisioning
> moving that function?

oh!  i forgot that it was in glstreamtextureimage -- originally, i think that it
was going to be a standalone helper.

hrm, that's annoying, then.  probably just forget the whole thing.  maybe switch
the initial matrix to be non-flipped identity, since i think that it's intended.

[DaleCurtis, 2016-09-30 22:55:50]

Done, please give it another look over and CQ if it looks good to you!

[liberato (no reviews please), 2016-10-05 15:35:16]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-10-05 15:35:17]

lgtm

https://codereview.chromium.org/2351293003/diff/60001/media/gpu/avda_shared_s...
File media/gpu/avda_shared_state.cc (right):

https://codereview.chromium.org/2351293003/diff/60001/media/gpu/avda_shared_s...
media/gpu/avda_shared_state.cc:22: 1, 0, 1, 0,  // Default to a sane guess just
in case we can't get the
methinks that the 1 in the third column should be zero.  the 1 in the fourth row
in the original is added by the y flip to translate along y.

i'll cq this and land a follow-up.

[commit-bot: I haz the power, 2016-10-05 15:35:56]

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, 2016-10-05 16:33:38]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-05 16:35:22]

Description was changed from

==========
Relocate SurfaceTexture usage from AVDA GLImage into shared state.

GetTransforMatrix is only correct for the most recent call to
UpdateTexImage(), which implies it should be shared among all
images instead of specific to each one. As such move it over to
the shared state.

Once done there's no other usage of SurfaceTexture in the image,
so we can also move UpdateTexImage over to the shared state to
avoid comments like "must call UpdateTransformMatrix after calling
UpdateTexImage... etc"

BUG=646603
TEST=video still plays
==========

to

==========
Relocate SurfaceTexture usage from AVDA GLImage into shared state.

GetTransforMatrix is only correct for the most recent call to
UpdateTexImage(), which implies it should be shared among all
images instead of specific to each one. As such move it over to
the shared state.

Once done there's no other usage of SurfaceTexture in the image,
so we can also move UpdateTexImage over to the shared state to
avoid comments like "must call UpdateTransformMatrix after calling
UpdateTexImage... etc"

BUG=646603
TEST=video still plays

Committed: https://crrev.com/304de87aa03df6fbbcafeee0983a7e74cf4e56ae
Cr-Commit-Position: refs/heads/master@{#423183}
==========

[commit-bot: I haz the power, 2016-10-05 16:35:23]

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