Run Demuxer::GetMemoryUsage() on media thread

Run Demuxer::GetMemoryUsage() on media thread

Split the memory usage reporting in WebMediaPlayerImpl into two steps so
that |demuxer_| can be accessed on the media thread only.

This change makes no practical difference for ChunkDemuxer ATM, which
uses locks extensively anyway.  But it does make a difference for
FFmpegDemuxer (as evidenced by
https://codereview.chromium.org/1419753007 -- can be reverted now).
Also, other Chromium-based products are better off if Demuxer
implementations are not forced to worry about synchronization
themselves.

BUG=564034
TEST=Crash page from 447898 still doesn't crash

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

[wdzierzanowski, 2015-12-01 15:57:48]

wdzierzanowski@opera.com changed reviewers:
+ dalecurtis@chromium.org

[wdzierzanowski, 2015-12-01 15:57:49]

PTAL

[DaleCurtis, 2015-12-01 18:15:37]

Will need to rebase, but lgtm otherwise. Are you going to handle the revert of
the other change after this?

[wdzierzanowski, 2015-12-01 18:18:52]

On 2015/12/01 18:15:37, DaleCurtis wrote:
> Will need to rebase, but lgtm otherwise.

Thanks for reviewing!

> Are you going to handle the revert of the other change after this?

Yup.

[wdzierzanowski, 2015-12-01 18:24:01]

Small comment update in Patch Set 2.

[wdzierzanowski, 2015-12-01 18:44:32]

Rebased in Patch Set 3.

[wdzierzanowski, 2015-12-01 19:48:32]

On 2015/12/01 18:44:32, wdzierzanowski wrote:
> Rebased in Patch Set 3.

Should I commit already (because it's LGTM-ed), or should I get another LGTM
after the rebase?

I've never had to rebase a Chromium CL, that's why I'm asking :)

[DaleCurtis, 2015-12-01 20:14:20]

Just realized this adds an extra thread hop, what do you think about avoiding
it?

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:90: // Returns the memory usage in bytes for the demuxer.
Seems this change should be reverted with the other patch set? Did you mean to
include it here?

https://codereview.chromium.org/1480213005/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1480213005/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1107: memory_usage_reporting_timer_.Start(
Should we run the timer on the media thread instead? Right now it's
RenderThread::PostDelayedTask()->MediaThread::PostTask()->RenderThread::PostTask()
versus MediaThread::PostDelayedTask()->RenderThread::PostTask().

[wdzierzanowski, 2015-12-02 09:27:54]

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:90: // Returns the memory usage in bytes for the demuxer.
On 2015/12/01 20:14:20, DaleCurtis wrote:
> Seems this change should be reverted with the other patch set? Did you mean to
> include it here?

I think it should be included here. This is the commit that lifts the
requirement on implementors to handle synchronization. After this change, it is
no longer true that this function "may be called from any thread".

https://codereview.chromium.org/1480213005/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1480213005/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1107: memory_usage_reporting_timer_.Start(
On 2015/12/01 20:14:20, DaleCurtis wrote:
> Should we run the timer on the media thread instead? Right now it's
>
RenderThread::PostDelayedTask()->MediaThread::PostTask()->RenderThread::PostTask()
> versus MediaThread::PostDelayedTask()->RenderThread::PostTask().

Interesting. I didn't think of that.

While we could indeed save one thread hop per each of the repeating memory
reports, it seems this would have to come at the cost of increased code
complexity:

  - The timer would have to be destroyed on the media thread as well.

  - We would no longer be able to use the handy PostTaskAndReplyWithResult()
utility. Instead, we would have to handle the sequence of calls on different
threads manually: The timer would call a WebMediaPlayerImpl function on the
media thread, which would post a task to the render thread.

  - With more pieces of WebMediaPlayerImpl state accessed on the media thread
there would come more instances of base::Unretained() uses requiring analysis of
threading issues. Some cases would become particularly hard, like checking for
existence of |demuxer_| on the media thread in a non-racy way given that it is
created on the renderer thread.

So I'm afraid in trying to save one thread hop in a piece of work that happens
once every 2 seconds we would make the implementation more difficult to follow
and error-prone.

[wdzierzanowski, 2015-12-02 09:31:41]

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:90: // Returns the memory usage in bytes for the demuxer.
On 2015/12/02 09:27:54, wdzierzanowski wrote:
> On 2015/12/01 20:14:20, DaleCurtis wrote:
> > Seems this change should be reverted with the other patch set? Did you mean
to
> > include it here?
> 
> I think it should be included here. This is the commit that lifts the
> requirement on implementors to handle synchronization. After this change, it
is
> no longer true that this function "may be called from any thread".

Ah, but you mean this will make 9ac642d1d2ed95b810ff276fb65c2be7b461e791
impossible to revert cleanly, right? OK, this convinces me :)

[wdzierzanowski, 2015-12-02 09:39:18]

I've also renamed StartMemoryUsageReport() back to ReportMemoryUsage(). The
former could wrongly suggest it's about starting the timer.

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:90: // Returns the memory usage in bytes for the demuxer.
On 2015/12/02 09:31:41, wdzierzanowski wrote:
> On 2015/12/02 09:27:54, wdzierzanowski wrote:
> > On 2015/12/01 20:14:20, DaleCurtis wrote:
> > > Seems this change should be reverted with the other patch set? Did you
mean
> to
> > > include it here?
> > 
> > I think it should be included here. This is the commit that lifts the
> > requirement on implementors to handle synchronization. After this change, it
> is
> > no longer true that this function "may be called from any thread".
> 
> Ah, but you mean this will make 9ac642d1d2ed95b810ff276fb65c2be7b461e791
> impossible to revert cleanly, right? OK, this convinces me :)

Done.

[DaleCurtis, 2015-12-02 17:11:36]

lgtm, this only happens every 2 seconds during playback now, so  I don't think
the ugliness is worth it.

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1480213005/diff/40001/media/base/demuxer.h#ne...
media/base/demuxer.h:90: // Returns the memory usage in bytes for the demuxer.
On 2015/12/02 09:39:17, wdzierzanowski wrote:
> On 2015/12/02 09:31:41, wdzierzanowski wrote:
> > On 2015/12/02 09:27:54, wdzierzanowski wrote:
> > > On 2015/12/01 20:14:20, DaleCurtis wrote:
> > > > Seems this change should be reverted with the other patch set? Did you
> mean
> > to
> > > > include it here?
> > > 
> > > I think it should be included here. This is the commit that lifts the
> > > requirement on implementors to handle synchronization. After this change,
it
> > is
> > > no longer true that this function "may be called from any thread".
> > 
> > Ah, but you mean this will make 9ac642d1d2ed95b810ff276fb65c2be7b461e791
> > impossible to revert cleanly, right? OK, this convinces me :)
> 
> Done.

Yes :)

[wdzierzanowski, 2015-12-02 20:21:20]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2015-12-02 20:24:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480213005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480213005/80001

[commit-bot: I haz the power, 2015-12-02 21:09:37]

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

[commit-bot: I haz the power, 2015-12-02 21:09:38]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wdzierzanowski, 2015-12-02 22:41:33]

I'm not convinced the failure is related. Retrying...

[wdzierzanowski, 2015-12-02 22:41:55]

The CQ bit was checked by wdzierzanowski@opera.com

[commit-bot: I haz the power, 2015-12-02 22:43:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480213005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480213005/80001

[commit-bot: I haz the power, 2015-12-02 23:50:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-02 23:51:16]

Description was changed from

==========
Run Demuxer::GetMemoryUsage() on media thread

Split the memory usage reporting in WebMediaPlayerImpl into two steps so
that |demuxer_| can be accessed on the media thread only.

This change makes no practical difference for ChunkDemuxer ATM, which
uses locks extensively anyway.  But it does make a difference for
FFmpegDemuxer (as evidenced by
https://codereview.chromium.org/1419753007 -- can be reverted now).
Also, other Chromium-based products are better off if Demuxer
implementations are not forced to worry about synchronization
themselves.

BUG=564034
TEST=Crash page from 447898 still doesn't crash
==========

to

==========
Run Demuxer::GetMemoryUsage() on media thread

Split the memory usage reporting in WebMediaPlayerImpl into two steps so
that |demuxer_| can be accessed on the media thread only.

This change makes no practical difference for ChunkDemuxer ATM, which
uses locks extensively anyway.  But it does make a difference for
FFmpegDemuxer (as evidenced by
https://codereview.chromium.org/1419753007 -- can be reverted now).
Also, other Chromium-based products are better off if Demuxer
implementations are not forced to worry about synchronization
themselves.

BUG=564034
TEST=Crash page from 447898 still doesn't crash

Committed: https://crrev.com/fd4cd91c5eea8b3a4970f5512a306e4a03e33101
Cr-Commit-Position: refs/heads/master@{#362821}
==========

[commit-bot: I haz the power, 2015-12-02 23:51:17]

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