Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves synchornization issues with this logic by bouncing 
ReplaceCurrentFrameWithACopy() calls off of IO thread so that they are 
scheduled after the last frame before pause is received.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.

Review-Url: https://codereview.chromium.org/2859993002
Cr-Commit-Position: refs/heads/master@{#471850}
Committed: https://chromium.googlesource.com/chromium/src/+/44cdf9e94c51e582a375fa8dfccbf4518898d63a

[emircan, 2017-05-04 00:01:44]

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

[commit-bot: I haz the power, 2017-05-04 00:03:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-05-04 00:08:34]

Description was changed from

==========
webms


fix copies

BUG=
==========

to

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves a couple issues with this logic:
- Bounces ReplaceCurrentFrameWithACopy() calls off of IO thread so that
they are scheduled after the last frame before pause is received.
- Releases the |current_frame_lock_| while the actual copy is happening so 
that other calls aren't blocked.
- Skips copy for texture backed frames as we are only concerned about the
buffering of local video capture.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.
==========

[emircan, 2017-05-04 00:08:34]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org, qiangchen@chromium.org

[emircan, 2017-05-04 00:08:46]

PTAL.

[DaleCurtis, 2017-05-04 00:45:37]

defer to qiangchen@ for review. I don't know this code very well.

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:409: if
(!current_frame_.get() || !player_)
Can anyone else set current_frame_? Or is this called from multiple threads? Not
safe to check unlock and relock then.

[commit-bot: I haz the power, 2017-05-04 01:05:23]

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

[commit-bot: I haz the power, 2017-05-04 01:05:24]

Dry run: This issue passed the CQ dry run.

[qiangchen, 2017-05-04 16:25:59]

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:43: if
(!frame->HasTextures()) {
What about frame with texture? This way, you will simply not make a copy of it.

[emircan, 2017-05-04 17:57:58]

Description was changed from

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves a couple issues with this logic:
- Bounces ReplaceCurrentFrameWithACopy() calls off of IO thread so that
they are scheduled after the last frame before pause is received.
- Releases the |current_frame_lock_| while the actual copy is happening so 
that other calls aren't blocked.
- Skips copy for texture backed frames as we are only concerned about the
buffering of local video capture.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.
==========

to

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves some issues with this logic:
- Bounces ReplaceCurrentFrameWithACopy() calls off of IO thread so that
they are scheduled after the last frame before pause is received.
- Skips copy for texture backed frames as we are only concerned about the
buffering of local video capture.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.
==========

[emircan, 2017-05-04 17:58:08]

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

[emircan, 2017-05-04 17:58:14]

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:43: if
(!frame->HasTextures()) {
On 2017/05/04 16:25:59, qiangchen wrote:
> What about frame with texture? This way, you will simply not make a copy of
it.

Yes, that is what I am intending to do. Look at the comment in l.341 before my
changes. It says we make copy because we have a limited number of video capture
buffers(3) and we don't want to hold onto one here. However, there is no texture
buffers originated from video capture. Local capture buffers are all mappable.
We download the texture buffers into CPU memory here and they are moved back to
textures before compositing, which is totally unnecessary.

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:409: if
(!current_frame_.get() || !player_)
On 2017/05/04 00:45:37, DaleCurtis_OOO_May_5_To_May23 wrote:
> Can anyone else set current_frame_? Or is this called from multiple threads?
Not
> safe to check unlock and relock then.

I was thinking of releasing the lock so that basic getters in this class can
work: GetCurrentSize(), total_frame_count(), dropped_frame_count(),
HasCurrentFrame() . But I realized they are all running on main thread as well,
so this won't help at all. I am reverting it so that lock applies to the whole
scope.

[commit-bot: I haz the power, 2017-05-04 17:58:42]

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, 2017-05-04 18:51:21]

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

[commit-bot: I haz the power, 2017-05-04 18:51:21]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-05-05 17:42:24]

Friendly ping qiangchen@.

[qiangchen, 2017-05-06 12:44:38]

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:43: if
(!frame->HasTextures()) {
On 2017/05/04 17:58:14, emircan wrote:
> On 2017/05/04 16:25:59, qiangchen wrote:
> > What about frame with texture? This way, you will simply not make a copy of
> it.
> 
> Yes, that is what I am intending to do. Look at the comment in l.341 before my
> changes. It says we make copy because we have a limited number of video
capture
> buffers(3) and we don't want to hold onto one here. However, there is no
texture
> buffers originated from video capture. Local capture buffers are all mappable.
> We download the texture buffers into CPU memory here and they are moved back
to
> textures before compositing, which is totally unnecessary.

But there is buffer limit for hardware decoder, I think the number is 4 if I
remember correctly. (You can test it by holding 4 frames in WMPMS, and see that
the stream is totally freezing) So at least, we need to copy the frame here and
release the old one. If you think the download upload is a performance concern,
what about copying it within GPU?

[emircan, 2017-05-08 17:04:40]

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

[commit-bot: I haz the power, 2017-05-08 17:04:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-05-08 17:05:29]

Description was changed from

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves some issues with this logic:
- Bounces ReplaceCurrentFrameWithACopy() calls off of IO thread so that
they are scheduled after the last frame before pause is received.
- Skips copy for texture backed frames as we are only concerned about the
buffering of local video capture.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.
==========

to

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves synchornization issues with this logic by bouncing 
ReplaceCurrentFrameWithACopy() calls off of IO thread so that they are 
scheduled after the last frame before pause is received.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.
==========

[emircan, 2017-05-08 17:14:02]

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2859993002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms_compositor.cc:43: if
(!frame->HasTextures()) {
On 2017/05/06 12:44:38, qiangchen wrote:
> On 2017/05/04 17:58:14, emircan wrote:
> > On 2017/05/04 16:25:59, qiangchen wrote:
> > > What about frame with texture? This way, you will simply not make a copy
of
> > it.
> > 
> > Yes, that is what I am intending to do. Look at the comment in l.341 before
my
> > changes. It says we make copy because we have a limited number of video
> capture
> > buffers(3) and we don't want to hold onto one here. However, there is no
> texture
> > buffers originated from video capture. Local capture buffers are all
mappable.
> > We download the texture buffers into CPU memory here and they are moved back
> to
> > textures before compositing, which is totally unnecessary.
> 
> But there is buffer limit for hardware decoder, I think the number is 4 if I
> remember correctly. (You can test it by holding 4 frames in WMPMS, and see
that
> the stream is totally freezing) So at least, we need to copy the frame here
and
> release the old one. If you think the download upload is a performance
concern,
> what about copying it within GPU?

That would be 4 frames per decoder though. So, it would cause a problem if
somebody decides to play the same decoded stream in 4+ players and pause at
different times. It is unlikely but I understand that we can't allow that case.
Thanks for pointing out. I am reverting those changes.
I couldn't find any texture to texture copy example using SKCanvasVideoRenderer
that can do it within GPU. I am not sure if we can do that from renderer.

[commit-bot: I haz the power, 2017-05-08 18:17:27]

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

[commit-bot: I haz the power, 2017-05-08 18:17:28]

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

[qiangchen, 2017-05-15 16:43:33]

lgtm

[emircan, 2017-05-15 16:56:45]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2017-05-15 16:57:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiangchen, 2017-05-15 16:59:41]

I am not the owner. You will need Dale's lgtm to commit.

[commit-bot: I haz the power, 2017-05-15 18:57:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494867405638010,
"parent_rev": "5764fa8dcfc1d110a7feaf39e26be8be583c1b54", "commit_rev":
"44cdf9e94c51e582a375fa8dfccbf4518898d63a"}

[commit-bot: I haz the power, 2017-05-15 18:57:59]

Description was changed from

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves synchornization issues with this logic by bouncing 
ReplaceCurrentFrameWithACopy() calls off of IO thread so that they are 
scheduled after the last frame before pause is received.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.
==========

to

==========
Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

This CL solves synchornization issues with this logic by bouncing 
ReplaceCurrentFrameWithACopy() calls off of IO thread so that they are 
scheduled after the last frame before pause is received.

BUG=715227
TEST=WebMediaPlayerMSTest.PlayThenPause* tests pass.

Review-Url: https://codereview.chromium.org/2859993002
Cr-Commit-Position: refs/heads/master@{#471850}
Committed:
https://chromium.googlesource.com/chromium/src/+/44cdf9e94c51e582a375fa8dfccb...
==========

[commit-bot: I haz the power, 2017-05-15 18:58:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/44cdf9e94c51e582a375fa8dfccb...