Add methods for telling V8 how much memory audio/video is using.

Add methods for telling V8 how much memory audio/video is using.

Adds GetMemoryUsage() methods to Demuxer and Pipeline.  Pipeline
returns the results via PipelineStatistics callbacks from the
audio and video renderers.

WebMediaPlayerImpl now recieves a method from the RenderFrameImpl
for talking to the V8 isolate and notifying it of memory changes.

This fixes a long standing issue of V8 not knowing how much memory
the media elements are soaking up.

BUG=447898
TEST=new unittests, original oom crash page doesn't crash.

Committed: https://crrev.com/83266c7462e414731592d6e7946de4ccb10f8a5a
Cr-Commit-Position: refs/heads/master@{#356900}

[DaleCurtis, 2015-10-24 00:31:24]

Description was changed from

==========
Add methods for telling V8 how much memory audio/video is using.

Adds GetMemoryUsage() methods to Demuxer and Pipeline.  Pipeline
returns the results via PipelineStatistics callbacks from the
audio and video renderers.

WebMediaPlayerImpl now recieves a method from the RenderFrameImpl
for talking to the V8 isolate and notifying it of memory changes.

BUG=447898
TEST=tbd
==========

to

==========
Add methods for telling V8 how much memory audio/video is using.

Adds GetMemoryUsage() methods to Demuxer and Pipeline.  Pipeline
returns the results via PipelineStatistics callbacks from the
audio and video renderers.

WebMediaPlayerImpl now recieves a method from the RenderFrameImpl
for talking to the V8 isolate and notifying it of memory changes.

This fixes a long standing issue of V8 not knowing how much memory
the media elements are soaking up.

BUG=447898
TEST=tbd
==========

[DaleCurtis, 2015-10-24 00:31:24]

dalecurtis@chromium.org changed reviewers:
+ ulan@chromium.org, wolenetz@chromium.org, xhwang@chromium.org

[DaleCurtis, 2015-10-24 00:33:52]

Will add some tests once we agree on the high level strategy. 

+xhwang,wolenetz: please review everything; wolenetz please sanity check my
ChunkDemuxer changes.

+ulan: please non-owner review render_frame_impl.cc and webmediaplayer_impl.cc
to make sure I'm sending sane data to
v8::Isolate::AdjustAmountOfExternalAllocatedMemory -- I think I might be missing
something about HandleScope.

cc: fs@opera as an FYI since he looked at this a while ago and may still be
interested in the outcome.

Thanks everyone!

[wolenetz, 2015-10-24 00:51:34]

cool stuff, Dale!
I only looked deeply at ChunkDemuxer; I'll look more later if you want. Nit:

https://codereview.chromium.org/1409123005/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1409123005/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:1313: int64_t ChunkDemuxer::GetMemoryUsage()
const {
While it would be interesting to see a machine that could validly have >
(2^(8*sizeof(size_t) - 1)) bytes per stream buffered, why does this method:
1) allow negative retval?
2) not just use size_t retval?

I suppose there's a degenerate case, also, where audio+video could overflow
size_t, too.

[DaleCurtis, 2015-10-24 00:55:56]

https://codereview.chromium.org/1409123005/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1409123005/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:1313: int64_t ChunkDemuxer::GetMemoryUsage()
const {
On 2015/10/24 00:51:34, wolenetz wrote:
> While it would be interesting to see a machine that could validly have >
> (2^(8*sizeof(size_t) - 1)) bytes per stream buffered, why does this method:
> 1) allow negative retval?
> 2) not just use size_t retval?
> 
> I suppose there's a degenerate case, also, where audio+video could overflow
> size_t, too.

I used int64_t since that's what v8 uses:

https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&l=...

The only other reason is so that I could use a -1 in the PipelineStatistics
class, but we could surface a size_t out instead everywhere except before we
hand into v8 w/ a checked cast.

[xhwang, 2015-10-25 17:01:56]

lgtm % nits

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/base/demux...
File media/base/demuxer.h (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/base/demux...
media/base/demuxer.h:90: virtual int64_t GetMemoryUsage() const = 0;
Return value should be in bytes?

How about add a comment?

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/base/pipel...
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/base/pipel...
media/base/pipeline.cc:51: // Pipeline clients should always see a zero value in
these cases.
In this case, why do we need the concept of "no data"? Can we just make the
default value to be 0?

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/base/pipel...
File media/base/pipeline_status.h (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/base/pipel...
media/base/pipeline_status.h:56: int64_t video_memory_usage;
nit: how about using the c++11 Non-Static Class Member Initializers here?

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/blink/webm...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/blink/webm...
media/blink/webmediaplayer_impl.cc:253: FROM_HERE,
base::TimeDelta::FromMilliseconds(500), this,
Is 500ms recommended by v8? I know nothing about this area but just want to
double check....

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/blink/webm...
media/blink/webmediaplayer_impl.cc:1110: adjust_allocated_memory_cb_.Run(delta);
If |delta| is zero or very small, should we still report? (Not sure what's the
cost of this call :)

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/blink/webm...
File media/blink/webmediaplayer_params.h (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/blink/webm...
media/blink/webmediaplayer_params.h:35: typedef base::Callback<int64_t(int64_t)>
AdjustAllocatedMemoryCB;
Please add a comment for this callback. It wasn't super clear to me what the
parameter is until I see how the callback is used. It's really reporting the
difference of memory usage since last report (hence "adjust"). For people not
familiar with the v8 code this isn't obvious.

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/filters/au...
File media/filters/audio_renderer_algorithm.h (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/filters/au...
media/filters/audio_renderer_algorithm.h:72: // Returns an estimate of amount of
memory used for buffered frames.
nit: s/memory/memory in bytes/

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/filters/vi...
File media/filters/video_renderer_algorithm.cc (right):

https://chromiumcodereview.appspot.com/1409123005/diff/20001/media/filters/vi...
media/filters/video_renderer_algorithm.cc:355: for (const auto& v :
frame_queue_) {
s/v/ready_frame/ ?

[wolenetz, 2015-10-26 16:49:38]

heads-up: I'm ooo sick today; will be back and able to review tomorrow in more
depth if needed: I only had a non-strong-feeling nit on media/filters/chunk*
previously, so am ok with lgtm that and rslgtm others' full review of the rest
of this CL. I would caution other reviewers to watch for deadlock possibility on
the taking of the lock by ChunkDemuxer (just do a quick scan of the change to
make sure it isn't possibly allowing some call to CD::GetMemoryUsage() from some
context where the CD lock is already held.

[ulan, 2015-10-27 12:02:26]

ulan@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[ulan, 2015-10-27 12:02:26]

https://codereview.chromium.org/1409123005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1409123005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2147:
base::Unretained(v8::Isolate::GetCurrent())),
v8::Isolate::GetCurrent() is going to be deprecated.
The V8 users should keep isolate and pass it explicitly.

I am not sure if it is possible to get to the isolate corresponding to the media
player from here.

Adding jochen@ and haraken@, who might know more.

[jochen (gone - plz use gerrit), 2015-10-27 12:03:54]

https://codereview.chromium.org/1409123005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1409123005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2147:
base::Unretained(v8::Isolate::GetCurrent())),
On 2015/10/27 at 12:02:26, ulan wrote:
> v8::Isolate::GetCurrent() is going to be deprecated.
> The V8 users should keep isolate and pass it explicitly.
> 
> I am not sure if it is possible to get to the isolate corresponding to the
media player from here.
> 
> Adding jochen@ and haraken@, who might know more.

just use blink::mainThreadIsolate() to get the main thread's isolate

[DaleCurtis, 2015-10-27 23:14:20]

jochen, ulan: PTAL.

https://codereview.chromium.org/1409123005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1409123005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2147:
base::Unretained(v8::Isolate::GetCurrent())),
On 2015/10/27 12:03:54, jochen wrote:
> On 2015/10/27 at 12:02:26, ulan wrote:
> > v8::Isolate::GetCurrent() is going to be deprecated.
> > The V8 users should keep isolate and pass it explicitly.
> > 
> > I am not sure if it is possible to get to the isolate corresponding to the
> media player from here.
> > 
> > Adding jochen@ and haraken@, who might know more.
> 
> just use blink::mainThreadIsolate() to get the main thread's isolate

Done.

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

https://codereview.chromium.org/1409123005/diff/20001/media/base/demuxer.h#ne...
media/base/demuxer.h:90: virtual int64_t GetMemoryUsage() const = 0;
On 2015/10/25 17:01:56, xhwang wrote:
> Return value should be in bytes?
> 
> How about add a comment?

Done.

https://codereview.chromium.org/1409123005/diff/20001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/1409123005/diff/20001/media/base/pipeline.cc#...
media/base/pipeline.cc:51: // Pipeline clients should always see a zero value in
these cases.
On 2015/10/25 17:01:56, xhwang wrote:
> In this case, why do we need the concept of "no data"? Can we just make the
> default value to be 0?

Removed in favor of returning delta stats from renderers.

https://codereview.chromium.org/1409123005/diff/20001/media/base/pipeline_sta...
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/1409123005/diff/20001/media/base/pipeline_sta...
media/base/pipeline_status.h:56: int64_t video_memory_usage;
On 2015/10/25 17:01:56, xhwang wrote:
> nit: how about using the c++11 Non-Static Class Member Initializers here?

Done.

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

https://codereview.chromium.org/1409123005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:253: FROM_HERE,
base::TimeDelta::FromMilliseconds(500), this,
On 2015/10/25 17:01:56, xhwang wrote:
> Is 500ms recommended by v8? I know nothing about this area but just want to
> double check....

Chosen arbitrarily. ulan@, jochen@ Does this value seem reasonable?

https://codereview.chromium.org/1409123005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1110: adjust_allocated_memory_cb_.Run(delta);
On 2015/10/25 17:01:56, xhwang wrote:
> If |delta| is zero or very small, should we still report? (Not sure what's the
> cost of this call :)

ulan@, jochen@: WDYT?

https://codereview.chromium.org/1409123005/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_params.h (right):

https://codereview.chromium.org/1409123005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_params.h:35: typedef base::Callback<int64_t(int64_t)>
AdjustAllocatedMemoryCB;
On 2015/10/25 17:01:56, xhwang wrote:
> Please add a comment for this callback. It wasn't super clear to me what the
> parameter is until I see how the callback is used. It's really reporting the
> difference of memory usage since last report (hence "adjust"). For people not
> familiar with the v8 code this isn't obvious.

Done.

https://codereview.chromium.org/1409123005/diff/20001/media/filters/audio_ren...
File media/filters/audio_renderer_algorithm.h (right):

https://codereview.chromium.org/1409123005/diff/20001/media/filters/audio_ren...
media/filters/audio_renderer_algorithm.h:72: // Returns an estimate of amount of
memory used for buffered frames.
On 2015/10/25 17:01:56, xhwang wrote:
> nit: s/memory/memory in bytes/

Done.

https://codereview.chromium.org/1409123005/diff/20001/media/filters/video_ren...
File media/filters/video_renderer_algorithm.cc (right):

https://codereview.chromium.org/1409123005/diff/20001/media/filters/video_ren...
media/filters/video_renderer_algorithm.cc:355: for (const auto& v :
frame_queue_) {
On 2015/10/25 17:01:56, xhwang wrote:
> s/v/ready_frame/ ?

Done.

[DaleCurtis, 2015-10-27 23:14:48]

Description was changed from

==========
Add methods for telling V8 how much memory audio/video is using.

Adds GetMemoryUsage() methods to Demuxer and Pipeline.  Pipeline
returns the results via PipelineStatistics callbacks from the
audio and video renderers.

WebMediaPlayerImpl now recieves a method from the RenderFrameImpl
for talking to the V8 isolate and notifying it of memory changes.

This fixes a long standing issue of V8 not knowing how much memory
the media elements are soaking up.

BUG=447898
TEST=tbd
==========

to

==========
Add methods for telling V8 how much memory audio/video is using.

Adds GetMemoryUsage() methods to Demuxer and Pipeline.  Pipeline
returns the results via PipelineStatistics callbacks from the
audio and video renderers.

WebMediaPlayerImpl now recieves a method from the RenderFrameImpl
for talking to the V8 isolate and notifying it of memory changes.

This fixes a long standing issue of V8 not knowing how much memory
the media elements are soaking up.

BUG=447898
TEST=new unittests, original oom crash page doesn't crash.
==========

[xhwang, 2015-10-27 23:32:40]

lgtm++

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline.cc#...
media/base/pipeline.cc:469: void Pipeline::OnUpdateStatistics(const
PipelineStatistics& stats) {
nit: In the future we should probably s/stats/stats_delta/

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline_sta...
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline_sta...
media/base/pipeline_status.h:41: // TODO(scherkus): this should be moved
alongside host interface definitions.
nit: Is this still accurate?

[DaleCurtis, 2015-10-28 01:08:28]

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline.cc#...
media/base/pipeline.cc:469: void Pipeline::OnUpdateStatistics(const
PipelineStatistics& stats) {
On 2015/10/27 23:32:40, xhwang wrote:
> nit: In the future we should probably s/stats/stats_delta/

Done.

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline_sta...
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/1409123005/diff/40001/media/base/pipeline_sta...
media/base/pipeline_status.h:41: // TODO(scherkus): this should be moved
alongside host interface definitions.
On 2015/10/27 23:32:40, xhwang wrote:
> nit: Is this still accurate?

Done.

[ulan, 2015-10-28 13:22:10]

lgtm

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

https://codereview.chromium.org/1409123005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:253: FROM_HERE,
base::TimeDelta::FromMilliseconds(500), this,
On 2015/10/27 23:14:20, DaleCurtis wrote:
> On 2015/10/25 17:01:56, xhwang wrote:
> > Is 500ms recommended by v8? I know nothing about this area but just want to
> > double check....
> 
> Chosen arbitrarily. ulan@, jochen@ Does this value seem reasonable?

Yes, but the value is not v8 specific so I am not sure if it is better than any
other value.

https://codereview.chromium.org/1409123005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1110: adjust_allocated_memory_cb_.Run(delta);
On 2015/10/27 23:14:20, DaleCurtis wrote:
> On 2015/10/25 17:01:56, xhwang wrote:
> > If |delta| is zero or very small, should we still report? (Not sure what's
the
> > cost of this call :)
> 
> ulan@, jochen@: WDYT?

It is fine to report.

Zero delta would be a no-op, the cost is function call + several memory
accesses.

[jochen (gone - plz use gerrit), 2015-10-28 14:41:26]

https://codereview.chromium.org/1409123005/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1409123005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:2146:
base::Unretained(blink::mainThreadIsolate())),
btw, it's important that you invoke this callback on the mainthread. can you
confirm that?

[haraken, 2015-10-28 14:45:36]

https://codereview.chromium.org/1409123005/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1409123005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:2146:
base::Unretained(blink::mainThreadIsolate())),
On 2015/10/28 14:41:26, jochen wrote:
> btw, it's important that you invoke this callback on the mainthread. can you
> confirm that?

mainThreadIsolate() has ASSERT(isMainThread()).

Or you can use toIsolate(Frame*). The fact that you have a frame means that
you're in the main thread.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[jochen (gone - plz use gerrit), 2015-10-28 14:47:20]

lgtm

[DaleCurtis, 2015-10-28 17:55:48]

Patchset #5 (id:80001) has been deleted

[DaleCurtis, 2015-10-28 17:56:01]

https://codereview.chromium.org/1409123005/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1409123005/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:2146:
base::Unretained(blink::mainThreadIsolate())),
On 2015/10/28 14:45:36, haraken wrote:
> On 2015/10/28 14:41:26, jochen wrote:
> > btw, it's important that you invoke this callback on the mainthread. can you
> > confirm that?
> 
> mainThreadIsolate() has ASSERT(isMainThread()).
> 
> Or you can use toIsolate(Frame*). The fact that you have a frame means that
> you're in the main thread.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Yup, main thread callbacks only. WMPI includes a
DCHECK(BelongsToCurrentThread()) too before this callback is executed. I've gone
ahead and added an extra comment in the WMPI code too.

[DaleCurtis, 2015-10-28 17:56:43]

Thanks for the reviews everyone!

[DaleCurtis, 2015-10-28 17:56:48]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-10-28 17:56:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org, xhwang@chromium.org, jochen@chromium.org,
ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/1409123005/#ps100001
(title: "Fix unittest compile error. Add comment.")

[commit-bot: I haz the power, 2015-10-28 17:57:53]

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

[commit-bot: I haz the power, 2015-10-28 18:20:37]

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

[commit-bot: I haz the power, 2015-10-28 18:20: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_...)

[DaleCurtis, 2015-10-29 00:07:00]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-10-29 00:07:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
ulan@chromium.org, xhwang@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1409123005/#ps120001
(title: "Fix html viewer.")

[commit-bot: I haz the power, 2015-10-29 00:08:29]

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

[commit-bot: I haz the power, 2015-10-29 01:00:56]

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

[commit-bot: I haz the power, 2015-10-29 01:00:56]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2015-10-29 15:30:23]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-10-29 15:32:05]

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

[commit-bot: I haz the power, 2015-10-29 17:42:04]

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

[commit-bot: I haz the power, 2015-10-29 17:42:06]

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

[DaleCurtis, 2015-10-29 17:46:28]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-10-29 17:47:43]

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

[commit-bot: I haz the power, 2015-10-29 18:43:27]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-10-29 18:44:01]

Patchset 6 (id:??) landed as
https://crrev.com/83266c7462e414731592d6e7946de4ccb10f8a5a
Cr-Commit-Position: refs/heads/master@{#356900}