New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 595827 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Track/analyze multibuffer memory usage

Project Member Reported by hubbe@chromium.org, Mar 17 2016

Issue description

Comment 1 by hubbe@chromium.org, Mar 18 2016

Apparently, field trials are activated only when you call base::FeatureList::IsEnabled(), which means that the use-new-media-cache will only get activated when playing a video, and on android, only if spitzer is on.

This means that the population in the above graph is 100% spitzer, which explains some things, but not everything. (Like, why don't we see this memory increase on other platforms?)



Comment 2 by hubbe@chromium.org, Mar 18 2016

It's possible that this CL: https://codereview.chromium.org/1754893006/ which was not merged back to M50 afaik, fixes the memory usage, which could explain why I don't see the issue on M51-CANARY-{M,W}

Do you see these issues on M50 desktop platforms?

Comment 4 by hubbe@chromium.org, Mar 18 2016

Looking...

Comment 5 by hubbe@chromium.org, Mar 22 2016

So I looked and I looked, but could not find any sign of this happening on desktop platforms. However, we now have enough M51-Android data that it looks like it is happening there.

My new theory is that the difference comes from differences in lifetime. The experiment is activated when we play a video, and the memory usage is expected to be slightly higher while playing a video (and for a short time after). On android, I suspect that the renderer is often killed shortly after playing the video, so in the UMA stat, we primarily see the time where the memory usage is higher. On other platforms, the renderer may live longer and we primarily see UMA stats where there is no difference.

Cc: asvitk...@chromium.org
Components: Internals>Media>Network
Labels: -Pri-2 Proj-Spitzer M-51 OS-Android Pri-1
But isn't this just comparing BufferedDataSource vs MultiBuffer? I.e. the Disabled group is only marked when a WebMediaPlayer is created with BufferedDataSource and the Enabled group is only marked when a WebMediaPlayer is created with MultiBuffer.

It follows that they should both have the same lifetime then right? I.e. any differences in renderer usage on desktop vs android should be moot since the comparison accounts for this.

cc:asvitkine in case he has any insight to add.

Comment 7 by hubbe@chromium.org, Mar 23 2016

My theory in #5 wasn't meant to explain why the memory usage goes up on android. It's meant to explain why it doesn't go up on other platforms. I suspect that the majority of the change in this graph is recorded after the video has ended, as BufferedDataSource would immediately free all the memory, while the multibuffer will free memory lazily. This might also explain why the graph has a somewhat odd shape.

I see. We should be able to verify that by nuking the lazy-release behavior for a release and seeing what the performance looks like.

Can you refresh my memory on how the lazy release works? I wasn't able to find the code off hand.

Comment 9 by hubbe@chromium.org, Mar 23 2016

Hmm, apparently, I can't find it either.
At some point I had piece of code that would call Prune() every now and then to free memory, but that seems to have been lost (or misplaced) in some revision.

Using more memory makes sense if it's not freed, but then, why doesn't it show up on other platforms?

Maybe renderers live longer? This would be consistent with the primarily single-tab browsing experience on Android.

It sounds like for now we should at least restore the pruning functionality.
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 29 2016

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

commit 6f09ac8211e36d634bd143881a870687bdb4c23f
Author: hubbe <hubbe@chromium.org>
Date: Tue Mar 29 00:45:52 2016

Lazily prune the multibuffer block cache.

Prunes ~5Mb per minute.

BUG= 595827 

Review URL: https://codereview.chromium.org/1829163002

Cr-Commit-Position: refs/heads/master@{#383636}

[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/base/BUILD.gn
[rename] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/base/fake_single_thread_task_runner.cc
[rename] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/base/fake_single_thread_task_runner.h
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/blink/multibuffer.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/blink/multibuffer.h
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/blink/multibuffer_data_source_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/blink/multibuffer_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/blink/url_index.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/blink/url_index_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/BUILD.gn
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/cast_testing.gypi
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/logging/encoding_event_subscriber_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/logging/receiver_time_offset_estimator_impl_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/logging/simple_event_subscriber_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/logging/stats_event_subscriber_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/net/cast_transport_sender_impl_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/net/pacing/paced_sender_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/net/rtcp/receiver_rtcp_event_subscriber_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/net/rtcp/rtcp_utility_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/net/rtp/rtp_packetizer_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/receiver/frame_receiver_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/sender/audio_encoder_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/sender/audio_sender_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/sender/congestion_control_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/sender/video_encoder_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/sender/video_sender_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/test/cast_benchmarks.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/test/end2end_unittest.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/cast/test/simulator.cc
[modify] https://crrev.com/6f09ac8211e36d634bd143881a870687bdb4c23f/media/media.gyp

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 8 2016

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

commit c3694d28aca6e32401771f6e9c47bb47a457fd3e
Author: hubbe <hubbe@chromium.org>
Date: Fri Apr 08 03:07:17 2016

Fix memory usage bug.

Don't tell multibuffer that it's ok to use 25Mb if we don't need it. Instead, use the bitrate to figure out what the max buffer should be (but clamp it to 25Mb).

BUG= 595827 

Review URL: https://codereview.chromium.org/1869203003

Cr-Commit-Position: refs/heads/master@{#385963}

[modify] https://crrev.com/c3694d28aca6e32401771f6e9c47bb47a457fd3e/media/blink/multibuffer_data_source.cc

Sign in to add a comment