New issue
Advanced search Search tips

Issue 715227 link

Starred by 1 user
Status: Fixed
Owner:
emir...@chromium.org
Closed: May 2017
Cc:
qiangchen@chromium.org
mflodman@chromium.org
sprang@chromium.org
brandtr@chromium.org
terelius@chromium.org
holmer@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

Project Member Reported by emir...@chromium.org, Apr 25 2017 
There are some problems with this logic since the refactor of passing frames to IO thread:
- While WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() is running on the main thread, WebMediaPlayerMSCompositor::EnqueueFrame() can be called on IO thread. As a result, the copied frame can be overwritten. We should serialize this.
- We do not need to hold onto the |current_frame_lock_| while making a copy. We can release and let other getters/setters work in the meantime.

Comment 1 by emir...@chromium.org, Apr 25 2017 

  // Copy the frame so that rendering can show the last received frame.
  // The original frame must not be referenced when the player is paused since
  // there might be a finite number of available buffers. E.g, video that
  // originates from a video camera.

Additionally, based on this comment, I think we should make a copy only when the buffer is mappable and I420, meaining the else case here.
https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms_compositor.cc?rcl=e4135c8d12a6554d16e3bf16a8b79bb6462aa7d8&l=89
Project Member

Comment 2 by bugdroid1@chromium.org, May 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44cdf9e94c51e582a375fa8dfccbf4518898d63a

commit 44cdf9e94c51e582a375fa8dfccbf4518898d63a
Author: emircan <emircan@chromium.org>
Date: Mon May 15 18:57:45 2017

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}

[modify] https://crrev.com/44cdf9e94c51e582a375fa8dfccbf4518898d63a/content/renderer/media/webmediaplayer_ms.cc
[modify] https://crrev.com/44cdf9e94c51e582a375fa8dfccbf4518898d63a/content/renderer/media/webmediaplayer_ms_compositor.cc
[modify] https://crrev.com/44cdf9e94c51e582a375fa8dfccbf4518898d63a/content/renderer/media/webmediaplayer_ms_compositor.h

Comment 3 by emir...@chromium.org, May 15 2017

Status: Fixed (was: Assigned)

Comment 4 by anatolid@chromium.org, May 31 2017

Labels: M-60
►

Sign in to add a comment