Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605993002:
Experiment with more aggressive MSE GC on memory pressure (Closed)
|
Created:
3 years, 11 months ago by servolk Modified:
3 years, 10 months ago CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExperiment with more aggressive MSE GC on memory pressure
Add an experimental ReduceMSEBuffersOnMemoryPressure feature that will
make MSE garbage collection algorithm more aggresive when we are under
moderate or critical memory pressure. Until now the MSE GC algorithm
didn't try very hard to free up memory as long as we are under
hard-coded memory cap (150MB for video, 12MB for audio). With this CL
the GC algorithm will halve the effective SourceBuffer memory limits
under moderate memory pressure and will release as much stale data as
possible under critical memory pressure.
BUG=672976
Review-Url: https://codereview.chromium.org/2605993002
Cr-Commit-Position: refs/heads/master@{#447245}
Committed: https://chromium.googlesource.com/chromium/src/+/f94b4608f14a23fcc7e1a1d0f7a01c4a6fca8bb0
Patch Set 1 #Patch Set 2 : Fix test failures #Patch Set 3 : Log total released bytes #Patch Set 4 : buildfix #Patch Set 5 : Fixed GC logic, use stream memory limit #Patch Set 6 : wip #Patch Set 7 : rebase #Patch Set 8 : wip #Patch Set 9 : Allow MSE GC to happen in EOS state now #
Total comments: 3
Patch Set 10 : Keep current memory level in SBS and use it during GC #Patch Set 11 : Make immediate GC optional + add tests #Patch Set 12 : Add separate feature for memory pressure + improve new tests #Patch Set 13 : Don't create mem pressure listener when the feature is disabled #
Total comments: 19
Patch Set 14 : Move memory listener creation closer to ChunkDemuxer creation #Patch Set 15 : updated tests #Patch Set 16 : typo #
Total comments: 5
Patch Set 17 : Improve feature handling (CR feedback) #
Total comments: 20
Patch Set 18 : CR feedback #
Total comments: 6
Patch Set 19 : nits #
Total comments: 6
Patch Set 20 : CR feedback #Patch Set 21 : rebase/resolve conflicts #Messages
Total messages: 140 (97 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
servolk@chromium.org changed reviewers: + chcunningham@chromium.org, wolenetz@google.com
servolk@chromium.org changed reviewers: + dalecurtis@chromium.org
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Why does WMPI need to know about memory pressure? Seems this could be directly integrated at the lowest levels.
On 2017/01/12 19:52:56, DaleCurtis wrote: > Why does WMPI need to know about memory pressure? Seems this could be directly > integrated at the lowest levels. The main reason I did it through WMPI is that we need to know the current playback position (media time) in order to decide how much data we can safely garbage-collect (we must stop GC when we reach the GOP containing the current playback position). In lower levels (e.g. ChunkDemuxer) we don't know the current media playback time, we know only the demuxer last read position, which might be significantly ahead of current playback position on platforms with deep media pipeline (like Chromecast). Another reason I though it might be useful in WMPI is that we might be able to optimize memory usage by FFmpegDemuxer as well, if we pass on those events to WMPI::data_source_. Although I haven't explored that yet.
servolk@chromium.org changed reviewers: + wolenetz@chromium.org - wolenetz@google.com
MSE v1 standard only allows GC during appendBuffer()'s first steps. Yet this CL does it any time there's >= moderate memory pressure. Apps expect the buffered ranges to remain stable between completed MSE appendBuffer and remove operations. It seems to me that moderate memory pressure may thus break app expectations. (Critical memory pressure probably led to broken/badly-performing app, so maybe "background" GC for critical memory pressure is more palatable without spec change.) And if "background" GC isn't the eventual route chosen, there's less motivation for having the listener in WMPI versus ChunkDemuxer. https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... media/filters/chunk_demuxer.h:276: // |bytes_released| is an optional output parameter, if non-null it will Where is this param decl'ed?
https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... media/filters/source_buffer_stream.cc:828: // Try removing data from the back of the SourceBuffer, until we reach the We may want to avoid removing data from the back in response to memory pressure. Its possible we may have a 2GB chromebook that is capable from GPU/CPU pov of playing 4K (or at least > 1080p). Such a machine will hit our current hard coded limit with under a minute of buffer and could easily face memory pressure. Cutting the limit in half (or restricting to current GOP) may cause these higher resolutions to be unwatchable (constant underflows due to tiny buffer ahead of current time).
Also, I looked at the memory UMAs from https://codereview.chromium.org/2568303002/ I don't have any surprising insights, but its worth looking at to get the ideas flowing. As expected demuxer memory is the big one. There is a cluster (~10%) of reports around the maximum limit (100 -155MB). I also note that we don't seprate demuxer memory from *chunk demuxer* memory. I.e. it will be harder to know the effects of this sort of change because the MSE UMA and Src= UMA are grouped together in one metric. Can you add a suffix to separate demuxer memory for MSE vs SRC=?
On 2017/01/12 22:21:49, wolenetz wrote: > MSE v1 standard only allows GC during appendBuffer()'s first steps. Yet this CL > does it any time there's >= moderate memory pressure. > > Apps expect the buffered ranges to remain stable between completed MSE > appendBuffer and remove operations. > > It seems to me that moderate memory pressure may thus break app expectations. > (Critical memory pressure probably led to broken/badly-performing app, so maybe > "background" GC for critical memory pressure is more palatable without spec > change.) > > And if "background" GC isn't the eventual route chosen, there's less motivation > for having the listener in WMPI versus ChunkDemuxer. > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... > File media/filters/chunk_demuxer.h (right): > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... > media/filters/chunk_demuxer.h:276: // |bytes_released| is an optional output > parameter, if non-null it will > Where is this param decl'ed? Good point, on MSE spec compliance, I haven't thought about that. I guess we could try a slightly different approach for handling the moderate memory pressure then. On moderate memory pressure instead of performing the GC immediately, we could instead remember in the ChunkDemuxer/ChunkDemuxerStream/SourceBufferStream that we are in the moderate memory pressure conditions and do a more aggressive GC during the next append. The downside of this approach, of course, is that the memory is going to be released slower, but I guess that might be still ok for moderate pressure. Although for critical memory pressure we probably still want to do the aggressive GC ASAP, as that's still likely to give better overall experience, which IMHO is worth the small risk of not being 100% compliant in this particular corner case.
https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... media/filters/chunk_demuxer.h:276: // |bytes_released| is an optional output parameter, if non-null it will On 2017/01/12 22:21:49, wolenetz wrote: > Where is this param decl'ed? Oops, I've copy/pasted this comment from ChunkDemuxerStream::EvictCodedFrames, where I've added the optional bytes_released parameter. I'll remove it here.
On 2017/01/12 22:31:26, chcunningham_ooo_jan11 wrote: > https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... > media/filters/source_buffer_stream.cc:828: // Try removing data from the back of > the SourceBuffer, until we reach the > We may want to avoid removing data from the back in response to memory pressure. > Its possible we may have a 2GB chromebook that is capable from GPU/CPU pov of > playing 4K (or at least > 1080p). Such a machine will hit our current hard coded > limit with under a minute of buffer and could easily face memory pressure. > Cutting the limit in half (or restricting to current GOP) may cause these higher > resolutions to be unwatchable (constant underflows due to tiny buffer ahead of > current time). I think I understand your reluctance to remove potentially useful data from the back, but I think it's still ok to do this. Consider a few points: 1. On Chromecast Ultra we are using 80MB MSE video buffers (that's all we can afford with 1GB of RAM, since we need to also allocate a lot of memory for decompressed 4K frames), which is only ~16-20 seconds of 4K video depending on bitrate, and yet that's enough for smooth 4K playback in Netflix, YouTube, Play Movies, Vudu. So chromebook with 2GB RAM should be fine too, even for 4K playback. No need to assume that you need to have a minute or more of video buffered ahead. In practice we've seen that being able to buffer ahead just ~10 sec is enough. 2. Note that we are going to stop removing data from the back when we reach the position of the last appended buffer, which means that any data between the current playback position and the most recent append position is protected from GC. Imagine a situation where you've been playing some video let's say at a position of ~1000 sec and then seeked all the way to the beginning of the stream, i.e. 0 sec. If the player relies on MSE GC it may not explicitly remove the old buffered data at ~1000 sec position and your buffered ranges will look e.g. like this [0; 10] [1000; 1090]. I'd argue that this future data past the most recent append position is worth sacrificing in most cases, and doubly so under memory pressure conditions.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/12 22:52:44, servolk wrote: > On 2017/01/12 22:21:49, wolenetz wrote: > > MSE v1 standard only allows GC during appendBuffer()'s first steps. Yet this > CL > > does it any time there's >= moderate memory pressure. > > > > Apps expect the buffered ranges to remain stable between completed MSE > > appendBuffer and remove operations. > > > > It seems to me that moderate memory pressure may thus break app expectations. > > (Critical memory pressure probably led to broken/badly-performing app, so > maybe > > "background" GC for critical memory pressure is more palatable without spec > > change.) > > > > And if "background" GC isn't the eventual route chosen, there's less > motivation > > for having the listener in WMPI versus ChunkDemuxer. > > > > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... > > File media/filters/chunk_demuxer.h (right): > > > > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/chunk_de... > > media/filters/chunk_demuxer.h:276: // |bytes_released| is an optional output > > parameter, if non-null it will > > Where is this param decl'ed? > > Good point, on MSE spec compliance, I haven't thought about that. I guess we > could try a slightly different approach for handling the moderate memory > pressure then. On moderate memory pressure instead of performing the GC > immediately, we could instead remember in the > ChunkDemuxer/ChunkDemuxerStream/SourceBufferStream that we are in the moderate > memory pressure conditions and do a more aggressive GC during the next append. > The downside of this approach, of course, is that the memory is going to be > released slower, but I guess that might be still ok for moderate pressure. > Although for critical memory pressure we probably still want to do the > aggressive GC ASAP, as that's still likely to give better overall experience, > which IMHO is worth the small risk of not being 100% compliant in this > particular corner case. Ok, I've made this change in patchset #10. Now we will keep the current memory pressure level in SourceBufferStream and based on the pressure status we will decide how much data will be released. We will still aim to halve the effective size of MSE buffers when under moderate pressure, and we'll collect as much data as possible when under critical pressure. But with this change we get two benefits: 1. Our behavior is now MSE spec compliant under moderate memory pressure. 2. Subsequent data appends while we are still under moderate memory pressure will be capped at half the regular stream memory limit. I've kept the behavior where we'll try to release as much memory as possible as soon as we are notified of critical memory pressure. I believe that the benefit of being able to release anywhere from 60 to 100MB or even more when the system is experiencing critical memory pressure could be very valuable, especially on platforms with 2GB of RAM or less. So I've kept the memory pressure listener in WMPI for now, since that's still where we can obtain the current playback time, which we are going need for the GC to run correctly. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think the critical memory pressure mode of background GC's unexpected changing of buffered memory from app perspective sounds like it needs a mechanism for notifying the app that GC has occurred. That background GC sounds like something for a separate CL that is done in cooperation with standards incubation so that applications and other implementors are on the same page.
On 2017/01/13 18:28:48, wolenetz wrote: > I think the critical memory pressure mode of background GC's unexpected changing > of buffered memory from app perspective sounds like it needs a mechanism for > notifying the app that GC has occurred. That background GC sounds like something > for a separate CL that is done in cooperation with standards incubation so that > applications and other implementors are on the same page. I agree that having such a mechanism would be nice, but IIUC MSE v1 spec is frozen and we can't add such a mechanism until v2, right? How about we make the background GC optional for now and launch it in experimental mode on Finch behind a command-line flag. Then we can gather some stats and see if this increases playback issues or not. I have a hunch that it will be an improvements for most apps and won't really break anything. WDYT?
On 2017/01/17 17:43:42, servolk wrote: > On 2017/01/13 18:28:48, wolenetz wrote: > > I think the critical memory pressure mode of background GC's unexpected > changing > > of buffered memory from app perspective sounds like it needs a mechanism for > > notifying the app that GC has occurred. That background GC sounds like > something > > for a separate CL that is done in cooperation with standards incubation so > that > > applications and other implementors are on the same page. > > I agree that having such a mechanism would be nice, but IIUC MSE v1 spec is > frozen and we can't add such a mechanism until v2, right? How about we make the > background GC optional for now and launch it in experimental mode on Finch > behind a command-line flag. Then we can gather some stats and see if this > increases playback issues or not. I have a hunch that it will be an improvements > for most apps and won't really break anything. WDYT? It's more a case of, V1 has shipped, any eventual standardized vNext API set-of-changes is likely to need to first pass narrower focused incubation. So getting a change like this incubated (e.g. WICG spec discussion with other implementors and web authors) successfully will be a good signal that this feature is ready for wider review for inclusion in vNext. I'm fine with experimenting, but what data will you be looking for to determine if the experiment is working or not?
On 2017/01/18 01:09:34, wolenetz wrote: > On 2017/01/17 17:43:42, servolk wrote: > > On 2017/01/13 18:28:48, wolenetz wrote: > > > I think the critical memory pressure mode of background GC's unexpected > > changing > > > of buffered memory from app perspective sounds like it needs a mechanism for > > > notifying the app that GC has occurred. That background GC sounds like > > something > > > for a separate CL that is done in cooperation with standards incubation so > > that > > > applications and other implementors are on the same page. > > > > I agree that having such a mechanism would be nice, but IIUC MSE v1 spec is > > frozen and we can't add such a mechanism until v2, right? How about we make > the > > background GC optional for now and launch it in experimental mode on Finch > > behind a command-line flag. Then we can gather some stats and see if this > > increases playback issues or not. I have a hunch that it will be an > improvements > > for most apps and won't really break anything. WDYT? > > It's more a case of, V1 has shipped, any eventual standardized vNext API > set-of-changes is likely to need to first pass narrower focused incubation. So > getting a change like this incubated (e.g. WICG spec discussion with other > implementors and web authors) successfully will be a good signal that this > feature is ready for wider review for inclusion in vNext. > > I'm fine with experimenting, but what data will you be looking for to determine > if the experiment is working or not? Also, before turning a feature on-by-default like background GC, we'll need launch approval which usually means that authors and other implementors have expressed interest in such support/implementing such support. The risk of shipping any feature in Chrome without such support or vetting is that it becomes defacto expected and lead to web platform fragmentation if other implementors make different decisions on the feature area.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/18 01:11:31, wolenetz wrote: > On 2017/01/18 01:09:34, wolenetz wrote: > > On 2017/01/17 17:43:42, servolk wrote: > > > On 2017/01/13 18:28:48, wolenetz wrote: > > > > I think the critical memory pressure mode of background GC's unexpected > > > changing > > > > of buffered memory from app perspective sounds like it needs a mechanism > for > > > > notifying the app that GC has occurred. That background GC sounds like > > > something > > > > for a separate CL that is done in cooperation with standards incubation so > > > that > > > > applications and other implementors are on the same page. > > > > > > I agree that having such a mechanism would be nice, but IIUC MSE v1 spec is > > > frozen and we can't add such a mechanism until v2, right? How about we make > > the > > > background GC optional for now and launch it in experimental mode on Finch > > > behind a command-line flag. Then we can gather some stats and see if this > > > increases playback issues or not. I have a hunch that it will be an > > improvements > > > for most apps and won't really break anything. WDYT? > > > > It's more a case of, V1 has shipped, any eventual standardized vNext API > > set-of-changes is likely to need to first pass narrower focused incubation. So > > getting a change like this incubated (e.g. WICG spec discussion with other > > implementors and web authors) successfully will be a good signal that this > > feature is ready for wider review for inclusion in vNext. > > > > I'm fine with experimenting, but what data will you be looking for to > determine > > if the experiment is working or not? > > Also, before turning a feature on-by-default like background GC, we'll need > launch approval which usually means that authors and other implementors have > expressed interest in such support/implementing such support. > The risk of shipping any feature in Chrome without such support or vetting is > that it becomes defacto expected and lead to web platform fragmentation if other > implementors make different decisions on the feature area. Ok, I've made the immediate-GC-on-memory-pressure an optional feature and have disabled it by default for now. We might want to enable it by default on low-memory platforms (e.g. Chromecast) and we might also create a Finch experiment that would enable this on some percentage of clients. We can then look at playback metrics (e.g. media pipeline errors or media watch time metrics) to see if this breaks thing or improves things. I've also added some tests in the latest patchset. PTAL.
On 2017/01/12 23:16:02, servolk wrote: > On 2017/01/12 22:31:26, chcunningham_ooo_jan11 wrote: > > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... > > File media/filters/source_buffer_stream.cc (right): > > > > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... > > media/filters/source_buffer_stream.cc:828: // Try removing data from the back > of > > the SourceBuffer, until we reach the > > We may want to avoid removing data from the back in response to memory > pressure. > > Its possible we may have a 2GB chromebook that is capable from GPU/CPU pov of > > playing 4K (or at least > 1080p). Such a machine will hit our current hard > coded > > limit with under a minute of buffer and could easily face memory pressure. > > Cutting the limit in half (or restricting to current GOP) may cause these > higher > > resolutions to be unwatchable (constant underflows due to tiny buffer ahead of > > current time). > > I think I understand your reluctance to remove potentially useful data from the > back, but I think it's still ok to do this. Consider a few points: > 1. On Chromecast Ultra we are using 80MB MSE video buffers (that's all we can > afford with 1GB of RAM, since we need to also allocate a lot of memory for > decompressed 4K frames), which is only ~16-20 seconds of 4K video depending on > bitrate, and yet that's enough for smooth 4K playback in Netflix, YouTube, Play > Movies, Vudu. So chromebook with 2GB RAM should be fine too, even for 4K > playback. No need to assume that you need to have a minute or more of video > buffered ahead. In practice we've seen that being able to buffer ahead just ~10 > sec is enough. > 2. Note that we are going to stop removing data from the back when we reach the > position of the last appended buffer, which means that any data between the > current playback position and the most recent append position is protected from > GC. Imagine a situation where you've been playing some video let's say at a > position of ~1000 sec and then seeked all the way to the beginning of the > stream, i.e. 0 sec. If the player relies on MSE GC it may not explicitly remove > the old buffered data at ~1000 sec position and your buffered ranges will look > e.g. like this [0; 10] [1000; 1090]. I'd argue that this future data past the > most recent append position is worth sacrificing in most cases, and doubly so > under memory pressure conditions. Thanks for explaining. This is fairly compelling but I'm still not _completely_ convinced. 10 seconds ahead should be fine if your network is fast and consistent - but thats not guaranteed. I expect that for many apps like Youtube, the strategy upon rebuffer is to increase the buffer-ahead size before continuing playback. It may be that 10 seconds is all they need, but I wouldn't be surprised to find out otherwise. We should loop in strobe@ for feedback. We should also probably disable the limit adjustments by default and enable only in a finch experiment with YT closely monitoring the performance. I'll pm strobe@ to comment.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/20 00:14:43, chcunningham wrote: > On 2017/01/12 23:16:02, servolk wrote: > > On 2017/01/12 22:31:26, chcunningham_ooo_jan11 wrote: > > > > > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... > > > File media/filters/source_buffer_stream.cc (right): > > > > > > > > > https://codereview.chromium.org/2605993002/diff/160001/media/filters/source_b... > > > media/filters/source_buffer_stream.cc:828: // Try removing data from the > back > > of > > > the SourceBuffer, until we reach the > > > We may want to avoid removing data from the back in response to memory > > pressure. > > > Its possible we may have a 2GB chromebook that is capable from GPU/CPU pov > of > > > playing 4K (or at least > 1080p). Such a machine will hit our current hard > > coded > > > limit with under a minute of buffer and could easily face memory pressure. > > > Cutting the limit in half (or restricting to current GOP) may cause these > > higher > > > resolutions to be unwatchable (constant underflows due to tiny buffer ahead > of > > > current time). > > > > I think I understand your reluctance to remove potentially useful data from > the > > back, but I think it's still ok to do this. Consider a few points: > > 1. On Chromecast Ultra we are using 80MB MSE video buffers (that's all we can > > afford with 1GB of RAM, since we need to also allocate a lot of memory for > > decompressed 4K frames), which is only ~16-20 seconds of 4K video depending on > > bitrate, and yet that's enough for smooth 4K playback in Netflix, YouTube, > Play > > Movies, Vudu. So chromebook with 2GB RAM should be fine too, even for 4K > > playback. No need to assume that you need to have a minute or more of video > > buffered ahead. In practice we've seen that being able to buffer ahead just > ~10 > > sec is enough. > > 2. Note that we are going to stop removing data from the back when we reach > the > > position of the last appended buffer, which means that any data between the > > current playback position and the most recent append position is protected > from > > GC. Imagine a situation where you've been playing some video let's say at a > > position of ~1000 sec and then seeked all the way to the beginning of the > > stream, i.e. 0 sec. If the player relies on MSE GC it may not explicitly > remove > > the old buffered data at ~1000 sec position and your buffered ranges will look > > e.g. like this [0; 10] [1000; 1090]. I'd argue that this future data past the > > most recent append position is worth sacrificing in most cases, and doubly so > > under memory pressure conditions. > > Thanks for explaining. This is fairly compelling but I'm still not _completely_ > convinced. 10 seconds ahead should be fine if your network is fast and > consistent - but thats not guaranteed. I expect that for many apps like Youtube, > the strategy upon rebuffer is to increase the buffer-ahead size before > continuing playback. It may be that 10 seconds is all they need, but I wouldn't > be surprised to find out otherwise. We should loop in strobe@ for feedback. We > should also probably disable the limit adjustments by default and enable only in > a finch experiment with YT closely monitoring the performance. I'll pm strobe@ > to comment. Ok, I've now made both parts of this CL optional features that are disabled by default. Enabling the kReduceMSEBuffersOnMemoryPressure feature will tune effective memory limits for regular MSE GC that happens on every append to allow more stale data to be released, and enabling the kMSEInstantGCOnMemoryPressure feature will allow the MSE GC to happen instantly on critical memory pressure, without waiting for the next append operation. PTAL.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Looking pretty good. Great tests. https://codereview.chromium.org/2605993002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1749: memory_pressure_listener_ = Do we want to make a new one every time the pipeline gets started? (e.g. if someone calls load() more than once?). why not constructor? Also, do you want to bother creating it when not media source? https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_state.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_state.cc:313: // takes up the most memory and that's where we can expect most savings. I agree about the savings, but I don't follow on the "first" part. Isn't the effect of doing it last the same - we will still try to adjust the limits / GC in the same way? Maybe I'm missing something https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream.cc:750: case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE: Maybe no-op is fine, but what made you decide not to instead restore the memory limit to its original value? https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4759: NewCodedFrameGroupAppend(0, 20, &kDataA); Can you use the string version of this API instead. I find the old version pretty hard to read. https://cs.chromium.org/chromium/src/media/filters/source_buffer_stream_unitt... https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4802: NewCodedFrameGroupAppend(20, 15, &kDataA); Ditto https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4810: NewCodedFrameGroupAppend(0, 20, &kDataA); Ditto
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2605993002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1749: memory_pressure_listener_ = On 2017/01/25 03:47:15, chcunningham wrote: > Do we want to make a new one every time the pipeline gets started? (e.g. if > someone calls load() more than once?). why not constructor? > > Also, do you want to bother creating it when not media source? I guess I wanted to avoid having the overhead of memory listener if a media element is created, but not started. E.g. if there's a <video> element in the page, but it's not autoplay. But it's not a big deal, I guess. We can move it to where we are creating the |chunk_demuxer_| for now. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_state.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_state.cc:313: // takes up the most memory and that's where we can expect most savings. On 2017/01/25 03:47:15, chcunningham wrote: > I agree about the savings, but I don't follow on the "first" part. Isn't the > effect of doing it last the same - we will still try to adjust the limits / GC > in the same way? Maybe I'm missing something This might make a difference when the instant GC on memory pressure is enabled (the kMSEInstantGCOnMemoryPressure feature). In that case we'll perform GC immediately inside the it.second->OnMemoryPressure invocation. And if the memory pressure is critical, I guess we better free up as much memory as quickly as possible. By performing GC on video buffers first we'll be able to relieve memory pressure quicker, than if we started with let's say audio buffers. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream.cc:750: case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE: On 2017/01/25 03:47:15, chcunningham wrote: > Maybe no-op is fine, but what made you decide not to instead restore the memory > limit to its original value? Tbh I'm not sure what you mean by 'restore the memory limit' - restore which limit? We are not changing |memory_limit_| at all, and the |effective_memory_limit| is a local variable, which is already initialized to the |memory_limit_| value. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4759: NewCodedFrameGroupAppend(0, 20, &kDataA); On 2017/01/25 03:47:15, chcunningham wrote: > Can you use the string version of this API instead. I find the old version > pretty hard to read. > > https://cs.chromium.org/chromium/src/media/filters/source_buffer_stream_unitt... Well, one big advantage of using this form of the API is that it allows us to be much more concise. If we used the string API it would be something like: NewCodedFrameGroupAppend("0K 10 20 30 40 50K 60 70 80 90 100K 110 120 130 140 150K 160 170 180 190 200K"); I personally prefer the shorter form and find it more readable in this case, since we don't need to draw that much attention to actual frame timestamps. Plus other GC-related tests also use mostly the short form API, so for consistency I've used it here too. But I can change the string version, if you have a strong preference to the longer form.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2605993002/diff/240001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1749: memory_pressure_listener_ = On 2017/01/25 18:06:47, servolk wrote: > On 2017/01/25 03:47:15, chcunningham wrote: > > Do we want to make a new one every time the pipeline gets started? (e.g. if > > someone calls load() more than once?). why not constructor? > > > > Also, do you want to bother creating it when not media source? > > I guess I wanted to avoid having the overhead of memory listener if a media > element is created, but not started. E.g. if there's a <video> element in the > page, but it's not autoplay. > But it's not a big deal, I guess. We can move it to where we are creating the > |chunk_demuxer_| for now. Acknowledged. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_state.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_state.cc:313: // takes up the most memory and that's where we can expect most savings. On 2017/01/25 18:06:47, servolk wrote: > On 2017/01/25 03:47:15, chcunningham wrote: > > I agree about the savings, but I don't follow on the "first" part. Isn't the > > effect of doing it last the same - we will still try to adjust the limits / GC > > in the same way? Maybe I'm missing something > > This might make a difference when the instant GC on memory pressure is enabled > (the kMSEInstantGCOnMemoryPressure feature). In that case we'll perform GC > immediately inside the it.second->OnMemoryPressure invocation. And if the memory > pressure is critical, I guess we better free up as much memory as quickly as > possible. By performing GC on video buffers first we'll be able to relieve > memory pressure quicker, than if we started with let's say audio buffers. I think this is fine, I just wanted to be sure I wasn't missing something subtle - like if doing GC of video_streams_ first was expected to reduce the aggressiveness of GC for audio/text. Seems not. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream.cc:750: case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE: On 2017/01/25 18:06:47, servolk wrote: > On 2017/01/25 03:47:15, chcunningham wrote: > > Maybe no-op is fine, but what made you decide not to instead restore the > memory > > limit to its original value? > > Tbh I'm not sure what you mean by 'restore the memory limit' - restore which > limit? We are not changing |memory_limit_| at all, and the > |effective_memory_limit| is a local variable, which is already initialized to > the |memory_limit_| value. My bad. I overlooked the local variable. Seems it is behaving as I wanted, i.e. upon memory_pressur_none we go back to GC'ing with our original level of aggressiveness. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4759: NewCodedFrameGroupAppend(0, 20, &kDataA); > NewCodedFrameGroupAppend("0K 10 20 30 40 50K 60 70 80 90 100K 110 120 130 140 > 150K 160 170 180 190 200K"); It doesn't have to be this huge. you can make your gops smaller and have less of them and still verify everything you're doing in this test. This API (and the associated CheckExpectedRangesByTimestamp()) is more readable and better at self documenting. It clearly lays out where the GOPs start and stop (I otherwise have to look up the impl of NewCodedFrameGroupAppend and then lookup the value for default key frame interval). This helps readers verify that the expected ranges after garbage collection preserved the correct GOP.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_state.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_state.cc:313: // takes up the most memory and that's where we can expect most savings. On 2017/01/25 22:21:56, chcunningham wrote: > On 2017/01/25 18:06:47, servolk wrote: > > On 2017/01/25 03:47:15, chcunningham wrote: > > > I agree about the savings, but I don't follow on the "first" part. Isn't the > > > effect of doing it last the same - we will still try to adjust the limits / > GC > > > in the same way? Maybe I'm missing something > > > > This might make a difference when the instant GC on memory pressure is enabled > > (the kMSEInstantGCOnMemoryPressure feature). In that case we'll perform GC > > immediately inside the it.second->OnMemoryPressure invocation. And if the > memory > > pressure is critical, I guess we better free up as much memory as quickly as > > possible. By performing GC on video buffers first we'll be able to relieve > > memory pressure quicker, than if we started with let's say audio buffers. > > I think this is fine, I just wanted to be sure I wasn't missing something subtle > - like if doing GC of video_streams_ first was expected to reduce the > aggressiveness of GC for audio/text. Seems not. The aggressiveness of GC for audio/text is going to be the same as for video. As I've explained in my previous comment, the order here matters only for cases when we do instant GC on memory pressure notifications. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream.cc:750: case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE: On 2017/01/25 22:21:57, chcunningham wrote: > On 2017/01/25 18:06:47, servolk wrote: > > On 2017/01/25 03:47:15, chcunningham wrote: > > > Maybe no-op is fine, but what made you decide not to instead restore the > > memory > > > limit to its original value? > > > > Tbh I'm not sure what you mean by 'restore the memory limit' - restore which > > limit? We are not changing |memory_limit_| at all, and the > > |effective_memory_limit| is a local variable, which is already initialized to > > the |memory_limit_| value. > > My bad. I overlooked the local variable. Seems it is behaving as I wanted, i.e. > upon memory_pressur_none we go back to GC'ing with our original level of > aggressiveness. Acknowledged. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4759: NewCodedFrameGroupAppend(0, 20, &kDataA); On 2017/01/25 22:21:57, chcunningham wrote: > > NewCodedFrameGroupAppend("0K 10 20 30 40 50K 60 70 80 90 100K 110 120 130 140 > > 150K 160 170 180 190 200K"); > > It doesn't have to be this huge. you can make your gops smaller and have less of > them and still verify everything you're doing in this test. > > This API (and the associated CheckExpectedRangesByTimestamp()) is more readable > and better at self documenting. It clearly lays out where the GOPs start and > stop (I otherwise have to look up the impl of NewCodedFrameGroupAppend and then > lookup the value for default key frame interval). This helps readers verify that > the expected ranges after garbage collection preserved the correct GOP. Done. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4802: NewCodedFrameGroupAppend(20, 15, &kDataA); On 2017/01/25 03:47:15, chcunningham wrote: > Ditto Done. https://codereview.chromium.org/2605993002/diff/240001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:4810: NewCodedFrameGroupAppend(0, 20, &kDataA); On 2017/01/25 03:47:15, chcunningham wrote: > Ditto Done.
Thanks! LGTM
https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... media/base/media_switches.cc:167: // Reduce memory limits for MSE SourceBuffers under memory pressure conditions. Probably would be easier to have a single feature with multiple values. https://codereview.chromium.org/2605993002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1176: DCHECK(base::FeatureList::IsEnabled(kReduceMSEBuffersOnMemoryPressure)); Seems this should include both features?
https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... media/base/media_switches.cc:167: // Reduce memory limits for MSE SourceBuffers under memory pressure conditions. On 2017/01/26 19:24:50, DaleCurtis wrote: > Probably would be easier to have a single feature with multiple values. IIUC the base::Feature is only binary (enabled/disabled), I don't see a way to use custom values in there. I guess we could use custom command-line param, but I'm not sure if that's better than reusing the standard base::Feature and base::Feature list functionality. https://codereview.chromium.org/2605993002/diff/300001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/300001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1176: DCHECK(base::FeatureList::IsEnabled(kReduceMSEBuffersOnMemoryPressure)); On 2017/01/26 19:24:50, DaleCurtis wrote: > Seems this should include both features? No, the kMSEInstantGCOnMemoryPressure may or may not be enabled here. If kMSEInstantGCOnMemoryPressure is enabled, then we'll perform GC immediately within this call. If kMSEInstantGCOnMemoryPressure is disabled, then we'll just remember the memory pressure state in the SourceBufferStream and that'll take effect on the next append operation.
https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... media/base/media_switches.cc:167: // Reduce memory limits for MSE SourceBuffers under memory pressure conditions. On 2017/01/26 at 19:38:31, servolk wrote: > On 2017/01/26 19:24:50, DaleCurtis wrote: > > Probably would be easier to have a single feature with multiple values. > > IIUC the base::Feature is only binary (enabled/disabled), I don't see a way to use custom values in there. I guess we could use custom command-line param, but I'm not sure if that's better than reusing the standard base::Feature and base::Feature list functionality. Sure it's binary only, but you can use variations to modify the feature, https://cs.chromium.org/chromium/src/components/variations/variations_associa... See an example of what we did for keyframe_distance here: https://cs.chromium.org/search/?q=keyframe_distance The key bit is this will allow you to do easier comparisons across your experiment groups. You can't accurate compare the two groups otherwise.
On 2017/01/26 19:48:33, DaleCurtis wrote: > https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... > File media/base/media_switches.cc (right): > > https://codereview.chromium.org/2605993002/diff/300001/media/base/media_switc... > media/base/media_switches.cc:167: // Reduce memory limits for MSE SourceBuffers > under memory pressure conditions. > On 2017/01/26 at 19:38:31, servolk wrote: > > On 2017/01/26 19:24:50, DaleCurtis wrote: > > > Probably would be easier to have a single feature with multiple values. > > > > IIUC the base::Feature is only binary (enabled/disabled), I don't see a way to > use custom values in there. I guess we could use custom command-line param, but > I'm not sure if that's better than reusing the standard base::Feature and > base::Feature list functionality. > > Sure it's binary only, but you can use variations to modify the feature, > > https://cs.chromium.org/chromium/src/components/variations/variations_associa... > > See an example of what we did for keyframe_distance here: > https://cs.chromium.org/search/?q=keyframe_distance > > The key bit is this will allow you to do easier comparisons across your > experiment groups. You can't accurate compare the two groups otherwise. Are you talking about max_keyframe_distance_ms / max_keyframe_distance_to_disable_background_video in WMPParams that was added in https://codereview.chromium.org/2628313002? Yes, I guess I can try to do the same here.
Yes, though I was hoping the bugs that required we need it passed in that way would have been fixed.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
servolk@chromium.org changed reviewers: + asvitkine@chromium.org
On 2017/01/26 20:17:12, DaleCurtis wrote: > Yes, though I was hoping the bugs that required we need it passed in that way > would have been fixed. +asvitkine@ crbug.com/681160 is not fixed yet, although the crbug.com/681695 that's been blocking is fixed recently, so there's progress. For now I've made the new parameter along the lines of what's been done for max_keyframe_distance_to_disable_background_video. PTAL at the patchset #17.
servolk@chromium.org changed reviewers: + dgozman@chromium.org
On 2017/01/26 22:24:17, servolk wrote: > On 2017/01/26 20:17:12, DaleCurtis wrote: > > Yes, though I was hoping the bugs that required we need it passed in that way > > would have been fixed. > > +asvitkine@ > crbug.com/681160 is not fixed yet, although the crbug.com/681695 that's been > blocking is fixed recently, so there's progress. For now I've made the new > parameter along the lines of what's been done for > max_keyframe_distance_to_disable_background_video. PTAL at the patchset #17. +dgozman@ for content/
On 2017/01/26 22:24:17, servolk wrote: > On 2017/01/26 20:17:12, DaleCurtis wrote: > > Yes, though I was hoping the bugs that required we need it passed in that way > > would have been fixed. > > +asvitkine@ > crbug.com/681160 is not fixed yet, although the crbug.com/681695 that's been > blocking is fixed recently, so there's progress. For now I've made the new > parameter along the lines of what's been done for > max_keyframe_distance_to_disable_background_video. PTAL at the patchset #17. All the main support should be done, it's just we've not moved the APIs for params from component/variations to base/metrics. The part that's left is moving the params code in https://cs.chromium.org/chromium/src/components/variations/variations_associa... to something like base/metrics/field_trial_params.h
Description was changed from ========== Perform more aggressive MSE GC on memory pressure ========== to ========== Experiment with more aggressive MSE GC on memory pressure Add an experimental ReduceMSEBuffersOnMemoryPressure feature that will make MSE garbage collection algorithm more aggresive when we are under moderate or critical memory pressure. Until now the MSE GC algorithm didn't try very hard to free up memory as long as we are under hard-coded memory cap (150MB for video, 12MB for audio). With this CL the GC algorithm will halve the effective SourceBuffer memory limits under moderate memory pressure and will release as much stale data as possible under critical memory pressure. BUG=672976 ==========
https://codereview.chromium.org/2605993002/diff/320001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2605993002/diff/320001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:553: // TODO(avayvod, asvitkine): Query the value directly when it is available in TODO(servolk? :) https://codereview.chromium.org/2605993002/diff/320001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:558: "mse_force_gc_on_memory_pressure", false); Not sure if we need two variations here or if the implicit split on true/false is sufficient. Defer to the UMA folk. https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... content/public/common/web_preferences.h:275: // When MSE garbage-collection on memory pressure is enabled, the "When memory pressure based garbage collection is enabled for MSE," https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... content/public/common/web_preferences.h:278: // append (slower, but more MSE-spec compliant). s/more/is/ https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... content/public/common/web_preferences.h:279: // TODO(avayvod, asvitkine): Query the value directly when it is available in s/avayvod/servolk/ https://codereview.chromium.org/2605993002/diff/320001/media/base/media_switc... File media/base/media_switches.h (right): https://codereview.chromium.org/2605993002/diff/320001/media/base/media_switc... media/base/media_switches.h:94: MEDIA_EXPORT extern const base::Feature kReduceMSEBuffersOnMemoryPressure; How about kMemoryPressureBasedSourceBufferGC? The variable names then become enable_instant_source_buffer_gc or is_source_buffer_gc_deferred (default true). https://codereview.chromium.org/2605993002/diff/320001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/320001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1179: if (chunk_demuxer_) { DCHECK(chunk_demuxer_) and just don't register the listener if it's not mse. https://codereview.chromium.org/2605993002/diff/320001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1182: media_task_runner_->PostTask( I'm still not convinced why you need to provide the currenTime to ChunkDemuxer? It knows the last read pts from the demuxer stream which should be close enough to currentTime() to not matter. https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... media/filters/source_buffer_stream.cc:704: // effect on the next SourceBuffer append operation. But if we are notified Comment talks about critical but if force is enabled it does it immediately. Comment is also worded in a speculative sense when the exact operation can be specified instead :) You should paragraph break at "Strictly speaking..." too and replace that text with "Per spec, MSE garbage collection must only occur during SourceBuffer append()" etc.
https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... media/filters/source_buffer_stream.cc:722: if (!base::FeatureList::IsEnabled(kReduceMSEBuffersOnMemoryPressure)) { Also drop {} from single line if statements (here and elsewhere).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2605993002/diff/320001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2605993002/diff/320001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:553: // TODO(avayvod, asvitkine): Query the value directly when it is available in On 2017/01/26 23:07:36, DaleCurtis wrote: > TODO(servolk? :) Done. https://codereview.chromium.org/2605993002/diff/320001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:558: "mse_force_gc_on_memory_pressure", false); On 2017/01/26 23:07:36, DaleCurtis wrote: > Not sure if we need two variations here or if the implicit split on true/false > is sufficient. Defer to the UMA folk. We need to have at least 3 states: 1. No action on memory pressure (the old behavior and the control group for the experiment). 2. More aggressive MSE GC, but delayed until the next SourceBuffer append. 3. More aggressive MSE GC that is forced to happen immediately on memory pressure notification, without waiting for the next append. https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... content/public/common/web_preferences.h:275: // When MSE garbage-collection on memory pressure is enabled, the On 2017/01/26 23:07:36, DaleCurtis wrote: > "When memory pressure based garbage collection is enabled for MSE," Done. https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... content/public/common/web_preferences.h:278: // append (slower, but more MSE-spec compliant). On 2017/01/26 23:07:36, DaleCurtis wrote: > s/more/is/ Done. https://codereview.chromium.org/2605993002/diff/320001/content/public/common/... content/public/common/web_preferences.h:279: // TODO(avayvod, asvitkine): Query the value directly when it is available in On 2017/01/26 23:07:36, DaleCurtis wrote: > s/avayvod/servolk/ Done. https://codereview.chromium.org/2605993002/diff/320001/media/base/media_switc... File media/base/media_switches.h (right): https://codereview.chromium.org/2605993002/diff/320001/media/base/media_switc... media/base/media_switches.h:94: MEDIA_EXPORT extern const base::Feature kReduceMSEBuffersOnMemoryPressure; On 2017/01/26 23:07:36, DaleCurtis wrote: > How about kMemoryPressureBasedSourceBufferGC? The variable names then become > enable_instant_source_buffer_gc or is_source_buffer_gc_deferred (default true). Done. https://codereview.chromium.org/2605993002/diff/320001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2605993002/diff/320001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1179: if (chunk_demuxer_) { On 2017/01/26 23:07:37, DaleCurtis wrote: > DCHECK(chunk_demuxer_) and just don't register the listener if it's not mse. Done. https://codereview.chromium.org/2605993002/diff/320001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1182: media_task_runner_->PostTask( On 2017/01/26 23:07:37, DaleCurtis wrote: > I'm still not convinced why you need to provide the currenTime to ChunkDemuxer? > It knows the last read pts from the demuxer stream which should be close enough > to currentTime() to not matter. The last read pts is not guaranteed to be close enough to currentTime(). Yes, it happens to be fairly close in case of standard Chrome's standard RendererImpl renderer. But for Chromecast we read data much further ahead of currentTime and feed it into hardware pipeline, to ensure hw decoder pipeline is never stalled (especially important on older 1st gen Chromecasts with slower single-core CPUs). Also even being pretty close is not good enough, currentTime might still be in the previous GOP while the read position has advanced to the next GOP. MSE spec says GC should preserve the GOP containing the currentTime(), not the last read position (which is invisible from the app level anyway). https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... media/filters/source_buffer_stream.cc:704: // effect on the next SourceBuffer append operation. But if we are notified On 2017/01/26 23:07:37, DaleCurtis wrote: > Comment talks about critical but if force is enabled it does it immediately. > Comment is also worded in a speculative sense when the exact operation can be > specified instead :) > > You should paragraph break at "Strictly speaking..." too and replace that text > with "Per spec, MSE garbage collection must only occur during SourceBuffer > append()" etc. Updated the comment and moved it closer to where we decide the value of |force_gc| in WMPI::OnMemoryPressure. https://codereview.chromium.org/2605993002/diff/320001/media/filters/source_b... media/filters/source_buffer_stream.cc:722: if (!base::FeatureList::IsEnabled(kReduceMSEBuffersOnMemoryPressure)) { On 2017/01/26 23:08:42, DaleCurtis wrote: > Also drop {} from single line if statements (here and elsewhere). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % some nits, again defer on variations to metrics folk. https://codereview.chromium.org/2605993002/diff/340001/media/filters/chunk_de... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/2605993002/diff/340001/media/filters/chunk_de... media/filters/chunk_demuxer.h:73: bool force_gc); s/force_gc/force_instant_gc/ since technically the method always forces gc. https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... media/filters/source_buffer_stream.cc:732: if (base::FeatureList::IsEnabled(kMemoryPressureBasedSourceBufferGC)) { Unnecessary? Since it won't be set otherwise. https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... File media/filters/source_buffer_stream.h (right): https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... media/filters/source_buffer_stream.h:439: base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level_ = Don't mix inline initialization if other variables in the class aren't using it.
https://codereview.chromium.org/2605993002/diff/340001/media/filters/chunk_de... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/2605993002/diff/340001/media/filters/chunk_de... media/filters/chunk_demuxer.h:73: bool force_gc); On 2017/01/27 00:58:23, DaleCurtis wrote: > s/force_gc/force_instant_gc/ since technically the method always forces gc. Done. https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... media/filters/source_buffer_stream.cc:732: if (base::FeatureList::IsEnabled(kMemoryPressureBasedSourceBufferGC)) { On 2017/01/27 00:58:23, DaleCurtis wrote: > Unnecessary? Since it won't be set otherwise. This is needed, because we invoke SourceBufferStream::OnMemoryPressure directly in tests, verifying the old expected GC behavior when the feature is disabled. https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... File media/filters/source_buffer_stream.h (right): https://codereview.chromium.org/2605993002/diff/340001/media/filters/source_b... media/filters/source_buffer_stream.h:439: base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level_ = On 2017/01/27 00:58:23, DaleCurtis wrote: > Don't mix inline initialization if other variables in the class aren't using it. Actually, this class is already mixed and most fields seem to be initialized inline, I guess due to having 3 overloaded constructors. Perhaps we should just make the rest of the fields inline-initialized here? I don't see any good reason why e.g. coded_frame_group_start_time_ or last_output_buffer_timestamp_ or max_interbuffer_distance_ are not inline-initialized. They are independent from the stream type and input decoder configs.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... File media/filters/source_buffer_state.h (right): https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... media/filters/source_buffer_state.h:79: void OnMemoryPressure( Comment? https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... File media/filters/source_buffer_stream.h (right): https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... media/filters/source_buffer_stream.h:97: void OnMemoryPressure( Comment? https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:176: bool GarbageCollect(base::TimeDelta media_time, int newDataSize) { new_data_size
https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... File media/filters/source_buffer_state.h (right): https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... media/filters/source_buffer_state.h:79: void OnMemoryPressure( On 2017/01/27 18:03:19, Alexei Svitkine (very slow) wrote: > Comment? Done. https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... File media/filters/source_buffer_stream.h (right): https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... media/filters/source_buffer_stream.h:97: void OnMemoryPressure( On 2017/01/27 18:03:19, Alexei Svitkine (very slow) wrote: > Comment? Done. https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... media/filters/source_buffer_stream_unittest.cc:176: bool GarbageCollect(base::TimeDelta media_time, int newDataSize) { On 2017/01/27 18:03:19, Alexei Svitkine (very slow) wrote: > new_data_size Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
servolk@chromium.org changed reviewers: + tsepez@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/28 02:23:57, servolk wrote: > https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... > File media/filters/source_buffer_state.h (right): > > https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... > media/filters/source_buffer_state.h:79: void OnMemoryPressure( > On 2017/01/27 18:03:19, Alexei Svitkine (very slow) wrote: > > Comment? > > Done. > > https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... > File media/filters/source_buffer_stream.h (right): > > https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... > media/filters/source_buffer_stream.h:97: void OnMemoryPressure( > On 2017/01/27 18:03:19, Alexei Svitkine (very slow) wrote: > > Comment? > > Done. > > https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... > File media/filters/source_buffer_stream_unittest.cc (right): > > https://codereview.chromium.org/2605993002/diff/360001/media/filters/source_b... > media/filters/source_buffer_stream_unittest.cc:176: bool > GarbageCollect(base::TimeDelta media_time, int newDataSize) { > On 2017/01/27 18:03:19, Alexei Svitkine (very slow) wrote: > > new_data_size > > Done. Also +tsepez@ for content/public/common/common_param_traits_macros.h changes
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Messages LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/30 18:13:44, Tom Sepez wrote: > Messages LGTM Thanks! Ping dgozman@ for content/ changes.
content lgtm
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, dalecurtis@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2605993002/#ps400001 (title: "rebase/resolve conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1485880600824700,
"parent_rev": "8ca955425af665573f7ad3210643bcb3486c69b5", "commit_rev":
"f94b4608f14a23fcc7e1a1d0f7a01c4a6fca8bb0"}
Message was sent while issue was closed.
Description was changed from ========== Experiment with more aggressive MSE GC on memory pressure Add an experimental ReduceMSEBuffersOnMemoryPressure feature that will make MSE garbage collection algorithm more aggresive when we are under moderate or critical memory pressure. Until now the MSE GC algorithm didn't try very hard to free up memory as long as we are under hard-coded memory cap (150MB for video, 12MB for audio). With this CL the GC algorithm will halve the effective SourceBuffer memory limits under moderate memory pressure and will release as much stale data as possible under critical memory pressure. BUG=672976 ========== to ========== Experiment with more aggressive MSE GC on memory pressure Add an experimental ReduceMSEBuffersOnMemoryPressure feature that will make MSE garbage collection algorithm more aggresive when we are under moderate or critical memory pressure. Until now the MSE GC algorithm didn't try very hard to free up memory as long as we are under hard-coded memory cap (150MB for video, 12MB for audio). With this CL the GC algorithm will halve the effective SourceBuffer memory limits under moderate memory pressure and will release as much stale data as possible under critical memory pressure. BUG=672976 Review-Url: https://codereview.chromium.org/2605993002 Cr-Commit-Position: refs/heads/master@{#447245} Committed: https://chromium.googlesource.com/chromium/src/+/f94b4608f14a23fcc7e1a1d0f7a0... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/f94b4608f14a23fcc7e1a1d0f7a0... |
