Fix race condition when accessing time_progressing_.

Fix race condition when accessing time_progressing_.

The |time_progressing_| variable should only be accessed via the
media thread, it can't be accessed under lock since it is set
during a call which may be reentrant to locked methods.

The fix is to use a proxy for |time_progressing_| when called from
other threads; luckily the inverse of |render_first_frame_and_stop_|
will suffice.

BUG=512371
TEST=media_unittests

Committed: https://crrev.com/a10468d1cf8729983532b6b0e2595722ea284fbe
Cr-Commit-Position: refs/heads/master@{#339817}

[DaleCurtis, 2015-07-21 19:01:07]

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

[DaleCurtis, 2015-07-21 19:01:30]

WDYT? I thought about adding a "if (belongs_to_current_thread)" but that seemed
uglier.

[xhwang, 2015-07-21 21:01:38]

looking good, one question about the "inverse of |render_first_frame_and_stop_|"
and some nits.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:217:
MaybeFireEndedCallback(!render_first_frame_and_stop_);
Can you explain a little bit more how "inverse of
|render_first_frame_and_stop_|" would suffice?

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:750: size_t
VideoRendererImpl::MaybeFireEndedCallback(bool time_progressing) {
This must be called under the lock, append _Locked to the file name, and check
lock_accquired in the function?

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.h (right):

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:38: // VideoRendererImpl creates its own
thread for the sole purpose of timing frame
This needs to be updated :)

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:175: bool sink_started_;
Add a comment that these two can only be accessed on the |tak_runner_|?

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:178: base::Lock lock_;
Now it's unfortunate that it's pretty hard to know which member needs to be
accessed under the |lock_|.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:282: // only be accessed on the media
thread.
This file doesn't know about the media thread. Replace it with something related
to the task runner?

[DaleCurtis, 2015-07-22 00:44:44]

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:217:
MaybeFireEndedCallback(!render_first_frame_and_stop_);
On 2015/07/21 21:01:38, xhwang wrote:
> Can you explain a little bit more how "inverse of
> |render_first_frame_and_stop_|" would suffice?

Done.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.cc:750: size_t
VideoRendererImpl::MaybeFireEndedCallback(bool time_progressing) {
On 2015/07/21 21:01:38, xhwang wrote:
> This must be called under the lock, append _Locked to the file name, and check
> lock_accquired in the function?

Done.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
File media/renderers/video_renderer_impl.h (right):

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:38: // VideoRendererImpl creates its own
thread for the sole purpose of timing frame
On 2015/07/21 21:01:38, xhwang wrote:
> This needs to be updated :)

I'll do so in the CL that removes the old pathway.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:175: bool sink_started_;
On 2015/07/21 21:01:38, xhwang wrote:
> Add a comment that these two can only be accessed on the |tak_runner_|?

Done.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:178: base::Lock lock_;
On 2015/07/21 21:01:38, xhwang wrote:
> Now it's unfortunate that it's pretty hard to know which member needs to be
> accessed under the |lock_|.

Yeah, I'll reflow this in the deletion CL.

https://codereview.chromium.org/1247973002/diff/1/media/renderers/video_rende...
media/renderers/video_renderer_impl.h:282: // only be accessed on the media
thread.
On 2015/07/21 21:01:38, xhwang wrote:
> This file doesn't know about the media thread. Replace it with something
related
> to the task runner?

Done.

[xhwang, 2015-07-22 00:47:00]

LGTM!

https://codereview.chromium.org/1247973002/diff/20001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1247973002/diff/20001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:225: // MaybeStopSinkAfterFirstPaint()
runs and the next Render() call comes in.
I hope we can simplify this a bit one day :)

[DaleCurtis, 2015-07-22 00:56:02]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-07-22 00:56:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1247973002/20001

[commit-bot: I haz the power, 2015-07-22 02:06:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-22 02:07:01]

Patchset 2 (id:??) landed as
https://crrev.com/a10468d1cf8729983532b6b0e2595722ea284fbe
Cr-Commit-Position: refs/heads/master@{#339817}