Report the SurfaceTexture tranform matrix to CC timely

Report the SurfaceTexture tranform matrix to CC timely

Send the transform matrix to CC early to be able to calculate the render passes
correctly.

BUG=581589

[jchen10, 2016-01-27 05:51:03]

Description was changed from

==========
Report the SurfaceTexture tranform matrix to CC timely

Send the transform matrix to CC early to be able to calculate the render passes
correctly.

BUG=581589
==========

to

==========
Report the SurfaceTexture tranform matrix to CC timely

Send the transform matrix to CC early to be able to calculate the render passes
correctly.

BUG=581589
==========

[jchen10, 2016-01-27 05:51:03]

jie.a.chen@intel.com changed reviewers:
+ ccameron@chromium.org, kbr@chromium.org, reveman@chromium.org,
sievers@chromium.org, tobiasjs@chromium.org

[jchen10, 2016-01-27 05:54:46]

Please kindly take a look! Thanks a lot!

[no sievers, 2016-01-27 19:05:21]

https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:183:
scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
This is not safe here (unlike the previous call site). |owner_stub_| might be
NULL.
But you can early-out for that.

https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:185:
surface_texture_->UpdateTexImage();
It's not really clear how this fixes it. It just reorders things but the
fundamental race of dequeue (which might change the transform matrix needed for
the current buffer) with pending draw commands in the cmdbuffer remains.

[jchen10, 2016-01-28 01:31:39]

https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:183:
scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
On 2016/01/27 19:05:20, sievers wrote:
> This is not safe here (unlike the previous call site). |owner_stub_| might be
> NULL.
> But you can early-out for that.

Done.

https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:185:
surface_texture_->UpdateTexImage();
On 2016/01/27 19:05:20, sievers wrote:
> It's not really clear how this fixes it. It just reorders things but the
> fundamental race of dequeue (which might change the transform matrix needed
for
> the current buffer) with pending draw commands in the cmdbuffer remains.

The current problematic work flow is like:
1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
2. StreamTexure sends the event to CC over IPC;
3. CC uses the default or previous transform matrix, which is not fresh, to
caculate RenderQuad for the VideoLayer, and issues the wrong GL commands to GPU
process;
4. GPU process executes the commands from CC, and asks StreamTexture to prepare
the video texture; 
5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
frame;
6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
matrix for the new video frame, and sends it to CC over IPC, which is already
late;

With this CL, the new work flow is:
1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
2. StreamTexure sends the event to CC over IPC;
3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
frame;
4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
matrix for the new video frame, and sends it to CC over IPC;
5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer, and
issues the right GL commands to GPU process;
6. GPU process executes the commands from CC, and asks StreamTexture to prepare
the video texture;
7. StreamTexture doesn't need to bother SurfaceTexture, as the new video frame
was already updated correctly in the step 3;

[no sievers, 2016-01-28 19:33:26]

On 2016/01/28 01:31:39, jchen10 wrote:
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> File content/common/gpu/stream_texture_android.cc (right):
> 
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> content/common/gpu/stream_texture_android.cc:183:
> scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
> On 2016/01/27 19:05:20, sievers wrote:
> > This is not safe here (unlike the previous call site). |owner_stub_| might
be
> > NULL.
> > But you can early-out for that.
> 
> Done.
> 
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> content/common/gpu/stream_texture_android.cc:185:
> surface_texture_->UpdateTexImage();
> On 2016/01/27 19:05:20, sievers wrote:
> > It's not really clear how this fixes it. It just reorders things but the
> > fundamental race of dequeue (which might change the transform matrix needed
> for
> > the current buffer) with pending draw commands in the cmdbuffer remains.
> 
> The current problematic work flow is like:
> 1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
> 2. StreamTexure sends the event to CC over IPC;
> 3. CC uses the default or previous transform matrix, which is not fresh, to
> caculate RenderQuad for the VideoLayer, and issues the wrong GL commands to
GPU
> process;
> 4. GPU process executes the commands from CC, and asks StreamTexture to
prepare
> the video texture; 
> 5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
> frame;
> 6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
> matrix for the new video frame, and sends it to CC over IPC, which is already
> late;
> 
> With this CL, the new work flow is:
> 1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
> 2. StreamTexure sends the event to CC over IPC;
> 3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
> frame;
> 4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
> matrix for the new video frame, and sends it to CC over IPC;
> 5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer, and
> issues the right GL commands to GPU process;
> 6. GPU process executes the commands from CC, and asks StreamTexture to
prepare
> the video texture;
> 7. StreamTexture doesn't need to bother SurfaceTexture, as the new video frame
> was already updated correctly in the step 3;

But in after 3. the next frameAvailable notification (and hence UpdateTexImage
which might imply a matrix change) races with 5.
Or in other words when the matrix changes you draw a frame with the mismatching
next frame's matrix vs. drawing the next frame with the previous frame's matrix.

Also see crbug.com/371500 for possible solutions.

Another solution based on your patch would be to additionally require the client
to acknowledge each frame after it's been drawn (through a cmdbuffer token or
IPC). And then use this ACK to block further calls to updateTexImage(). But that
almost seems guaranteed to break pipelining and hurt performance (between the
compositor and gpu threads).

[no sievers, 2016-01-28 19:35:04]

On 2016/01/28 19:33:26, sievers wrote:
> On 2016/01/28 01:31:39, jchen10 wrote:
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > File content/common/gpu/stream_texture_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > content/common/gpu/stream_texture_android.cc:183:
> > scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
> > On 2016/01/27 19:05:20, sievers wrote:
> > > This is not safe here (unlike the previous call site). |owner_stub_| might
> be
> > > NULL.
> > > But you can early-out for that.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > content/common/gpu/stream_texture_android.cc:185:
> > surface_texture_->UpdateTexImage();
> > On 2016/01/27 19:05:20, sievers wrote:
> > > It's not really clear how this fixes it. It just reorders things but the
> > > fundamental race of dequeue (which might change the transform matrix
needed
> > for
> > > the current buffer) with pending draw commands in the cmdbuffer remains.
> > 
> > The current problematic work flow is like:
> > 1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
> > 2. StreamTexure sends the event to CC over IPC;
> > 3. CC uses the default or previous transform matrix, which is not fresh, to
> > caculate RenderQuad for the VideoLayer, and issues the wrong GL commands to
> GPU
> > process;
> > 4. GPU process executes the commands from CC, and asks StreamTexture to
> prepare
> > the video texture; 
> > 5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
> > frame;
> > 6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
> > matrix for the new video frame, and sends it to CC over IPC, which is
already
> > late;
> > 
> > With this CL, the new work flow is:
> > 1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
> > 2. StreamTexure sends the event to CC over IPC;
> > 3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
> > frame;
> > 4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
> > matrix for the new video frame, and sends it to CC over IPC;
> > 5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer,
and
> > issues the right GL commands to GPU process;
> > 6. GPU process executes the commands from CC, and asks StreamTexture to
> prepare
> > the video texture;
> > 7. StreamTexture doesn't need to bother SurfaceTexture, as the new video
frame
> > was already updated correctly in the step 3;
> 
> But in after 3. the next frameAvailable notification (and hence UpdateTexImage
> which might imply a matrix change) races with 5.

Actually I should say it doesn't even race just with 5. but the commands
generated in that step reaching the GPU thread.

[jchen10, 2016-01-29 02:37:34]

On 2016/01/28 19:35:04, sievers wrote:
> On 2016/01/28 19:33:26, sievers wrote:
> > On 2016/01/28 01:31:39, jchen10 wrote:
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > File content/common/gpu/stream_texture_android.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > content/common/gpu/stream_texture_android.cc:183:
> > > scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
> > > On 2016/01/27 19:05:20, sievers wrote:
> > > > This is not safe here (unlike the previous call site). |owner_stub_|
might
> > be
> > > > NULL.
> > > > But you can early-out for that.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > content/common/gpu/stream_texture_android.cc:185:
> > > surface_texture_->UpdateTexImage();
> > > On 2016/01/27 19:05:20, sievers wrote:
> > > > It's not really clear how this fixes it. It just reorders things but the
> > > > fundamental race of dequeue (which might change the transform matrix
> needed
> > > for
> > > > the current buffer) with pending draw commands in the cmdbuffer remains.
> > > 
> > > The current problematic work flow is like:
> > > 1. StreamTexture gets notified of FrameAvailable event from
SurfaceTexture;
> > > 2. StreamTexure sends the event to CC over IPC;
> > > 3. CC uses the default or previous transform matrix, which is not fresh,
to
> > > caculate RenderQuad for the VideoLayer, and issues the wrong GL commands
to
> > GPU
> > > process;
> > > 4. GPU process executes the commands from CC, and asks StreamTexture to
> > prepare
> > > the video texture; 
> > > 5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new
video
> > > frame;
> > > 6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the
right
> > > matrix for the new video frame, and sends it to CC over IPC, which is
> already
> > > late;
> > > 
> > > With this CL, the new work flow is:
> > > 1. StreamTexture gets notified of FrameAvailable event from
SurfaceTexture;
> > > 2. StreamTexure sends the event to CC over IPC;
> > > 3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new
video
> > > frame;
> > > 4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the
right
> > > matrix for the new video frame, and sends it to CC over IPC;
> > > 5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer,
> and
> > > issues the right GL commands to GPU process;
> > > 6. GPU process executes the commands from CC, and asks StreamTexture to
> > prepare
> > > the video texture;
> > > 7. StreamTexture doesn't need to bother SurfaceTexture, as the new video
> frame
> > > was already updated correctly in the step 3;
> > 
> > But in after 3. the next frameAvailable notification (and hence
UpdateTexImage
> > which might imply a matrix change) races with 5.
> 
> Actually I should say it doesn't even race just with 5. but the commands
> generated in that step reaching the GPU thread.

Got it!
The race is more complicated than I thought. The solution you mentioned in the
new
 media pipeline have perfectly solved this issue. So when the new pipeline is
supposed
 to be officially enabled? Will it be cherry-picked to M47, M48?

[jchen10, 2016-01-29 05:46:51]

On 2016/01/28 19:33:26, sievers wrote:
> On 2016/01/28 01:31:39, jchen10 wrote:
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > File content/common/gpu/stream_texture_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > content/common/gpu/stream_texture_android.cc:183:
> > scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
> > On 2016/01/27 19:05:20, sievers wrote:
> > > This is not safe here (unlike the previous call site). |owner_stub_| might
> be
> > > NULL.
> > > But you can early-out for that.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > content/common/gpu/stream_texture_android.cc:185:
> > surface_texture_->UpdateTexImage();
> > On 2016/01/27 19:05:20, sievers wrote:
> > > It's not really clear how this fixes it. It just reorders things but the
> > > fundamental race of dequeue (which might change the transform matrix
needed
> > for
> > > the current buffer) with pending draw commands in the cmdbuffer remains.
> > 
> > The current problematic work flow is like:
> > 1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
> > 2. StreamTexure sends the event to CC over IPC;
> > 3. CC uses the default or previous transform matrix, which is not fresh, to
> > caculate RenderQuad for the VideoLayer, and issues the wrong GL commands to
> GPU
> > process;
> > 4. GPU process executes the commands from CC, and asks StreamTexture to
> prepare
> > the video texture; 
> > 5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
> > frame;
> > 6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
> > matrix for the new video frame, and sends it to CC over IPC, which is
already
> > late;
> > 
> > With this CL, the new work flow is:
> > 1. StreamTexture gets notified of FrameAvailable event from SurfaceTexture;
> > 2. StreamTexure sends the event to CC over IPC;
> > 3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new video
> > frame;
> > 4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the right
> > matrix for the new video frame, and sends it to CC over IPC;
> > 5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer,
and
> > issues the right GL commands to GPU process;
> > 6. GPU process executes the commands from CC, and asks StreamTexture to
> prepare
> > the video texture;
> > 7. StreamTexture doesn't need to bother SurfaceTexture, as the new video
frame
> > was already updated correctly in the step 3;
> 
> But in after 3. the next frameAvailable notification (and hence UpdateTexImage
> which might imply a matrix change) races with 5.
> Or in other words when the matrix changes you draw a frame with the
mismatching
> next frame's matrix vs. drawing the next frame with the previous frame's
matrix.
> 
> Also see crbug.com/371500 for possible solutions.
> 
> Another solution based on your patch would be to additionally require the
client
> to acknowledge each frame after it's been drawn (through a cmdbuffer token or
> IPC). And then use this ACK to block further calls to updateTexImage(). But
that
> almost seems guaranteed to break pipelining and hurt performance (between the
> compositor and gpu threads).

If the next frame has a different matrix, this CL will run into error. Actually
I haven't found such kind of device which changes its matrix on every frame. So
at least this CL can solve the first frame error, which can cover most existing
platforms. Do you think it's still useful in this sense?

[no sievers, 2016-01-29 18:28:22]

On 2016/01/29 05:46:51, jchen10 wrote:
> On 2016/01/28 19:33:26, sievers wrote:
> > On 2016/01/28 01:31:39, jchen10 wrote:
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > File content/common/gpu/stream_texture_android.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > content/common/gpu/stream_texture_android.cc:183:
> > > scoped_ptr<ui::ScopedMakeCurrent> scoped_make_current(MakeStubCurrent());
> > > On 2016/01/27 19:05:20, sievers wrote:
> > > > This is not safe here (unlike the previous call site). |owner_stub_|
might
> > be
> > > > NULL.
> > > > But you can early-out for that.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > content/common/gpu/stream_texture_android.cc:185:
> > > surface_texture_->UpdateTexImage();
> > > On 2016/01/27 19:05:20, sievers wrote:
> > > > It's not really clear how this fixes it. It just reorders things but the
> > > > fundamental race of dequeue (which might change the transform matrix
> needed
> > > for
> > > > the current buffer) with pending draw commands in the cmdbuffer remains.
> > > 
> > > The current problematic work flow is like:
> > > 1. StreamTexture gets notified of FrameAvailable event from
SurfaceTexture;
> > > 2. StreamTexure sends the event to CC over IPC;
> > > 3. CC uses the default or previous transform matrix, which is not fresh,
to
> > > caculate RenderQuad for the VideoLayer, and issues the wrong GL commands
to
> > GPU
> > > process;
> > > 4. GPU process executes the commands from CC, and asks StreamTexture to
> > prepare
> > > the video texture; 
> > > 5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new
video
> > > frame;
> > > 6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the
right
> > > matrix for the new video frame, and sends it to CC over IPC, which is
> already
> > > late;
> > > 
> > > With this CL, the new work flow is:
> > > 1. StreamTexture gets notified of FrameAvailable event from
SurfaceTexture;
> > > 2. StreamTexure sends the event to CC over IPC;
> > > 3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new
video
> > > frame;
> > > 4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the
right
> > > matrix for the new video frame, and sends it to CC over IPC;
> > > 5. CC uses new transform matrix to caculate RenderQuad for the VideoLayer,
> and
> > > issues the right GL commands to GPU process;
> > > 6. GPU process executes the commands from CC, and asks StreamTexture to
> > prepare
> > > the video texture;
> > > 7. StreamTexture doesn't need to bother SurfaceTexture, as the new video
> frame
> > > was already updated correctly in the step 3;
> > 
> > But in after 3. the next frameAvailable notification (and hence
UpdateTexImage
> > which might imply a matrix change) races with 5.
> > Or in other words when the matrix changes you draw a frame with the
> mismatching
> > next frame's matrix vs. drawing the next frame with the previous frame's
> matrix.
> > 
> > Also see crbug.com/371500 for possible solutions.
> > 
> > Another solution based on your patch would be to additionally require the
> client
> > to acknowledge each frame after it's been drawn (through a cmdbuffer token
or
> > IPC). And then use this ACK to block further calls to updateTexImage(). But
> that
> > almost seems guaranteed to break pipelining and hurt performance (between
the
> > compositor and gpu threads).
> 
> If the next frame has a different matrix, this CL will run into error.
Actually
> I haven't found such kind of device which changes its matrix on every frame.
So
> at least this CL can solve the first frame error, which can cover most
existing
> platforms. Do you think it's still useful in this sense?

Sounds good, but can you send the matrix before the invalidation then?

[jchen10, 2016-02-01 06:30:37]

On 2016/01/29 18:28:22, sievers wrote:
> On 2016/01/29 05:46:51, jchen10 wrote:
> > On 2016/01/28 19:33:26, sievers wrote:
> > > On 2016/01/28 01:31:39, jchen10 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > > File content/common/gpu/stream_texture_android.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > > content/common/gpu/stream_texture_android.cc:183:
> > > > scoped_ptr<ui::ScopedMakeCurrent>
scoped_make_current(MakeStubCurrent());
> > > > On 2016/01/27 19:05:20, sievers wrote:
> > > > > This is not safe here (unlike the previous call site). |owner_stub_|
> might
> > > be
> > > > > NULL.
> > > > > But you can early-out for that.
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1639183002/diff/20001/content/common/gpu/stre...
> > > > content/common/gpu/stream_texture_android.cc:185:
> > > > surface_texture_->UpdateTexImage();
> > > > On 2016/01/27 19:05:20, sievers wrote:
> > > > > It's not really clear how this fixes it. It just reorders things but
the
> > > > > fundamental race of dequeue (which might change the transform matrix
> > needed
> > > > for
> > > > > the current buffer) with pending draw commands in the cmdbuffer
remains.
> > > > 
> > > > The current problematic work flow is like:
> > > > 1. StreamTexture gets notified of FrameAvailable event from
> SurfaceTexture;
> > > > 2. StreamTexure sends the event to CC over IPC;
> > > > 3. CC uses the default or previous transform matrix, which is not fresh,
> to
> > > > caculate RenderQuad for the VideoLayer, and issues the wrong GL commands
> to
> > > GPU
> > > > process;
> > > > 4. GPU process executes the commands from CC, and asks StreamTexture to
> > > prepare
> > > > the video texture; 
> > > > 5. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new
> video
> > > > frame;
> > > > 6. StreamTexture calls SurfaceTexture's getTransformMatrix to get the
> right
> > > > matrix for the new video frame, and sends it to CC over IPC, which is
> > already
> > > > late;
> > > > 
> > > > With this CL, the new work flow is:
> > > > 1. StreamTexture gets notified of FrameAvailable event from
> SurfaceTexture;
> > > > 2. StreamTexure sends the event to CC over IPC;
> > > > 3. StreamTexture calls SurfaceTexture's UpdateTexImage to get the new
> video
> > > > frame;
> > > > 4. StreamTexture calls SurfaceTexture's getTransformMatrix to get the
> right
> > > > matrix for the new video frame, and sends it to CC over IPC;
> > > > 5. CC uses new transform matrix to caculate RenderQuad for the
VideoLayer,
> > and
> > > > issues the right GL commands to GPU process;
> > > > 6. GPU process executes the commands from CC, and asks StreamTexture to
> > > prepare
> > > > the video texture;
> > > > 7. StreamTexture doesn't need to bother SurfaceTexture, as the new video
> > frame
> > > > was already updated correctly in the step 3;
> > > 
> > > But in after 3. the next frameAvailable notification (and hence
> UpdateTexImage
> > > which might imply a matrix change) races with 5.
> > > Or in other words when the matrix changes you draw a frame with the
> > mismatching
> > > next frame's matrix vs. drawing the next frame with the previous frame's
> > matrix.
> > > 
> > > Also see crbug.com/371500 for possible solutions.
> > > 
> > > Another solution based on your patch would be to additionally require the
> > client
> > > to acknowledge each frame after it's been drawn (through a cmdbuffer token
> or
> > > IPC). And then use this ACK to block further calls to updateTexImage().
But
> > that
> > > almost seems guaranteed to break pipelining and hurt performance (between
> the
> > > compositor and gpu threads).
> > 
> > If the next frame has a different matrix, this CL will run into error.
> Actually
> > I haven't found such kind of device which changes its matrix on every frame.
> So
> > at least this CL can solve the first frame error, which can cover most
> existing
> > platforms. Do you think it's still useful in this sense?
> 
> Sounds good, but can you send the matrix before the invalidation then?

Not quite understood on the "send the matrix before the invalidation". It'd be
nice of you to give me more information, so the newbie may know the direction to
investigate.

[no sievers, 2016-02-01 18:30:47]

https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:199: owner_stub_->channel()->Send(
I meant: Send this message before GpuStreamTextureMsg_FrameAvailable above to
make sure it uses the correct matrix.

[jchen10, 2016-02-02 03:19:24]

https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:199: owner_stub_->channel()->Send(
On 2016/02/01 18:30:47, sievers wrote:
> I meant: Send this message before GpuStreamTextureMsg_FrameAvailable above to
> make sure it uses the correct matrix.

So nice! :)

Done.

[liberato (no reviews please), 2016-02-02 15:49:43]

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

[liberato (no reviews please), 2016-02-02 15:49:44]

optimistic?  maybe!

-fl

https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
content/common/gpu/stream_texture_android.cc:184: new
GpuStreamTextureMsg_FrameAvailable(route_id_));
if one moves this later (after matrixchanged), does it remove the race?

these are both handled by a listener bound to the CC thread.  how can they race
then?

[liberato (no reviews please), 2016-02-02 15:54:55]

On 2016/02/02 15:49:44, liberato wrote:
> optimistic?  maybe!
> 
> -fl
> 
>
https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
> File content/common/gpu/stream_texture_android.cc (right):
> 
>
https://codereview.chromium.org/1639183002/diff/40001/content/common/gpu/stre...
> content/common/gpu/stream_texture_android.cc:184: new
> GpuStreamTextureMsg_FrameAvailable(route_id_));
> if one moves this later (after matrixchanged), does it remove the race?
> 
> these are both handled by a listener bound to the CC thread.  how can they
race
> then?

sorry, had a stale copy of the review up when i sent that.  i missed several
days' worth of comments.

thanks
-fl

[Tobias Sargeant, 2016-02-02 16:02:05]

> if one moves this later (after matrixchanged), does it remove the race?
> 
> these are both handled by a listener bound to the CC thread.  how can they
race
> then?

I think not, because cc can prepare the RenderQuad using a matrix for a frame
that has already been overwritten. The GPU thread would need to know that the
compositor had received and processed the new matrix before updating the texture
or issuing a new OnFrameAvailable in order to avoid the race.

[liberato (no reviews please), 2016-02-02 16:16:30]

On 2016/02/02 16:02:05, Tobias Sargeant wrote:
> > if one moves this later (after matrixchanged), does it remove the race?
> > 
> > these are both handled by a listener bound to the CC thread.  how can they
> race
> > then?
> 
> I think not, because cc can prepare the RenderQuad using a matrix for a frame
> that has already been overwritten. The GPU thread would need to know that the
> compositor had received and processed the new matrix before updating the
texture
> or issuing a new OnFrameAvailable in order to avoid the race.

true.  i didn't state my assumption that it's (usually) constant.  mid-stream
format changes would be a good candidate to break this, i suppose.

[no sievers, 2016-02-02 18:43:32]

On 2016/02/02 16:16:30, liberato wrote:
> On 2016/02/02 16:02:05, Tobias Sargeant wrote:
> > > if one moves this later (after matrixchanged), does it remove the race?
> > > 
> > > these are both handled by a listener bound to the CC thread.  how can they
> > race
> > > then?
> > 
> > I think not, because cc can prepare the RenderQuad using a matrix for a
frame
> > that has already been overwritten. The GPU thread would need to know that
the
> > compositor had received and processed the new matrix before updating the
> texture
> > or issuing a new OnFrameAvailable in order to avoid the race.
> 
> true.  i didn't state my assumption that it's (usually) constant.  mid-stream
> format changes would be a good candidate to break this, i suppose.

Yes, this doesn't really fix anything reliably, see above. but i could see how
it might make it less likely that we use the wrong matrix for the first frame
(or when it changes mid-stream). wdyt?

To get it really right we'd have to do a lot more work, see crbug.com/371500.

[no sievers, 2016-02-19 20:37:05]

Take a look at https://codereview.chromium.org/1559203003 and TODO in
https://codereview.chromium.org/1559203003/diff/160001/cc/output/gl_renderer.cc.

This should fix your issue, I think.