Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2170303002:
Make MSE buffer sizes configurable via command line (Closed)
|
Created:
4 years, 5 months ago by servolk Modified:
4 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake MSE buffer sizes configurable via command line
BUG=630342
Committed: https://crrev.com/9cc90fd8941a3a2bd5328ff2841ae312b1ffe5b9
Cr-Commit-Position: refs/heads/master@{#409072}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Progapage the new switches from browser to renderer process + fix #
Total comments: 7
Patch Set 3 : CR feedback #
Total comments: 11
Patch Set 4 : Adjusted variable and switch names #
Dependent Patchsets: Messages
Total messages: 36 (19 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: This issue passed the CQ dry run.
Description was changed from ========== Make MSE buffer sizes configurable via command line BUG=630342 ========== to ========== Make MSE buffer sizes configurable via command line BUG=630342 ==========
servolk@chromium.org changed reviewers: + alokp@chromium.org, wolenetz@chromium.org
On 2016/07/22 18:10:09, servolk wrote: ping
Thanks for putting this together. Looking pretty good, though is this working (and how can we tell?) https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kMSEAudioBufferSize[]; IIUC, these need to be propogated from browser to renderer process (e.g., in https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...). Have you tested these flags? Can such test be automated? https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... media/filters/media_source_state.cc:587: std::string audioBufferSwitchStr = nit: I think the convention is to first check HasSwitch (here and below). https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... media/filters/media_source_state.cc:593: audio_->SetStreamMemoryLimit(mseAudioBufferSize); nit: Please add a MEDIA_LOG either here, or perhaps in SourceBufferStream to log the actual memory limit that was set. (Ditto for the video_ limit, below).
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kMSEAudioBufferSize[]; On 2016/07/28 17:55:08, wolenetz wrote: > IIUC, these need to be propogated from browser to renderer process (e.g., in > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...). > > Have you tested these flags? Can such test be automated? Good point, yeah, I'll add the new flags to that list. I haven't done much testing on this yet, I first wanted to see if you would be ok with making this configurable via command-line in the first place. As for the automated testing - I guess we could append those switches to the command line, but that will affect all media tests. I'm not sure if that's a good idea. On the other hand we do already have test coverage for the DemuxerStream::SetStreamMemoryLimit being invoked through MediaSourceState::SetMemoryLimits in chunk demuxer, SourceStreamBuffer and pipeline integration tests, so perhaps that's enough and we don't need to cover the command line switches specifically. WDYT? https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... media/filters/media_source_state.cc:587: std::string audioBufferSwitchStr = On 2016/07/28 17:55:08, wolenetz wrote: > nit: I think the convention is to first check HasSwitch (here and below). This comment seems to imply that it's unnecessary: https://cs.chromium.org/chromium/src/base/command_line.h?rcl=1469719166&l=166 https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... media/filters/media_source_state.cc:593: audio_->SetStreamMemoryLimit(mseAudioBufferSize); On 2016/07/28 17:55:08, wolenetz wrote: > nit: Please add a MEDIA_LOG either here, or perhaps in SourceBufferStream to log > the actual memory limit that was set. > > (Ditto for the video_ limit, below). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
looking good % nits and suggested new layout test in a special subfolder and virtual test suite. Also % OWNER review of content/browser/renderer_host/render_process_host_impl.cc For posterity, I like the idea of using at least a cmdline flag to limit the max stream buffer size, to assist platform customization. If/when we do dynamic limit adjustments, we can still retain such a custom limit as a maximum. WDYT? https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kMSEAudioBufferSize[]; On 2016/07/28 21:46:46, servolk wrote: > On 2016/07/28 17:55:08, wolenetz wrote: > > IIUC, these need to be propogated from browser to renderer process (e.g., in > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...). > > > > Have you tested these flags? Can such test be automated? > > Good point, yeah, I'll add the new flags to that list. I haven't done much > testing on this yet, I first wanted to see if you would be ok with making this > configurable via command-line in the first place. > As for the automated testing - I guess we could append those switches to the > command line, but that will affect all media tests. I'm not sure if that's a > good idea. On the other hand we do already have test coverage for the > DemuxerStream::SetStreamMemoryLimit being invoked through > MediaSourceState::SetMemoryLimits in chunk demuxer, SourceStreamBuffer and > pipeline integration tests, so perhaps that's enough and we don't need to cover > the command line switches specifically. WDYT? Let's not do this for all media tests. There is a VirtualTestSuite layout test functionality which could be used to run a test with a special set of flags and mark it [ Skip ] in TestExpectations to keep it from running on any but the intended platform+flag virtual variant. See https://codereview.chromium.org/1644763002/diff/160001 for an example. (Would probably require a subfolder under http/tests/media/media-source/ to contain this specific "test-memory-limit-command-line" test, but seems doable and valuable to automate.) https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/1/media/filters/media_source_... media/filters/media_source_state.cc:587: std::string audioBufferSwitchStr = On 2016/07/28 21:46:46, servolk wrote: > On 2016/07/28 17:55:08, wolenetz wrote: > > nit: I think the convention is to first check HasSwitch (here and below). > > This comment seems to imply that it's unnecessary: > https://cs.chromium.org/chromium/src/base/command_line.h?rcl=1469719166&l=166 Acknowledged. https://codereview.chromium.org/2170303002/diff/20001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2170303002/diff/20001/media/base/media_switch... media/base/media_switches.cc:134: const char kMSEAudioBufferSize[] = "mse-audio-buffer-size"; nit ditto: (include "limit" in the flag text) https://codereview.chromium.org/2170303002/diff/20001/media/base/media_switch... File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/20001/media/base/media_switch... media/base/media_switches.h:72: MEDIA_EXPORT extern const char kMSEVideoBufferSize[]; nit: rename to "kMSE{Audio,Video}BufferSizeLimit" https://codereview.chromium.org/2170303002/diff/20001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/20001/media/filters/media_sou... media/filters/media_source_state.cc:602: unsigned mseAudioBufferSize = 0; nit: include "Limit" even in this local identifier, here and in video code below. https://codereview.chromium.org/2170303002/diff/20001/media/filters/media_sou... media/filters/media_source_state.cc:605: MEDIA_LOG(INFO, media_log_) << "MSE audio SourceBuffer size=" nit: include "Custom" before "MSE audio So....", here and in video code below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
servolk@chromium.org changed reviewers: + sievers@chromium.org
+sievers@ for content/browser/renderer_host/render_process_host_impl.cc https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kMSEAudioBufferSize[]; On 2016/07/28 23:02:15, wolenetz wrote: > On 2016/07/28 21:46:46, servolk wrote: > > On 2016/07/28 17:55:08, wolenetz wrote: > > > IIUC, these need to be propogated from browser to renderer process (e.g., in > > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...). > > > > > > Have you tested these flags? Can such test be automated? > > > > Good point, yeah, I'll add the new flags to that list. I haven't done much > > testing on this yet, I first wanted to see if you would be ok with making this > > configurable via command-line in the first place. > > As for the automated testing - I guess we could append those switches to the > > command line, but that will affect all media tests. I'm not sure if that's a > > good idea. On the other hand we do already have test coverage for the > > DemuxerStream::SetStreamMemoryLimit being invoked through > > MediaSourceState::SetMemoryLimits in chunk demuxer, SourceStreamBuffer and > > pipeline integration tests, so perhaps that's enough and we don't need to > cover > > the command line switches specifically. WDYT? > > Let's not do this for all media tests. There is a VirtualTestSuite layout test > functionality which could be used to run a test with a special set of flags and > mark it [ Skip ] in TestExpectations to keep it from running on any but the > intended platform+flag virtual variant. See > https://codereview.chromium.org/1644763002/diff/160001 for an example. (Would > probably require a subfolder under http/tests/media/media-source/ to contain > this specific "test-memory-limit-command-line" test, but seems doable and > valuable to automate.) Ok, yeah, I'll look into adding a virtual test suite-based test. https://codereview.chromium.org/2170303002/diff/20001/media/base/media_switch... File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/20001/media/base/media_switch... media/base/media_switches.h:72: MEDIA_EXPORT extern const char kMSEVideoBufferSize[]; On 2016/07/28 23:02:15, wolenetz wrote: > nit: rename to "kMSE{Audio,Video}BufferSizeLimit" Done. https://codereview.chromium.org/2170303002/diff/20001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/20001/media/filters/media_sou... media/filters/media_source_state.cc:602: unsigned mseAudioBufferSize = 0; On 2016/07/28 23:02:16, wolenetz wrote: > nit: include "Limit" even in this local identifier, here and in video code > below. Done. https://codereview.chromium.org/2170303002/diff/20001/media/filters/media_sou... media/filters/media_source_state.cc:605: MEDIA_LOG(INFO, media_log_) << "MSE audio SourceBuffer size=" On 2016/07/28 23:02:16, wolenetz wrote: > nit: include "Custom" before "MSE audio So....", here and in video code below. Done.
content lgtm
https://codereview.chromium.org/2170303002/diff/40001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2170303002/diff/40001/media/base/media_switch... media/base/media_switches.cc:134: const char kMSEAudioBufferSizeLimit[] = "mse-audio-buffer-size"; nit: mse-audio-buffer-size-limit https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:598: if (audio_stream_just_created) { can you just move this into "if (!audio_)" block above? why do you need the audio_stream_just_created boolean? https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:599: std::string audioBufferSwitchStr = variable names do not follow chromium style guide. https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:602: unsigned mseAudioBufferSizeLimit = 0; variable name https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:622: bool video_stream_just_created = false; ditto
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2170303002/diff/40001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2170303002/diff/40001/media/base/media_switch... media/base/media_switches.cc:134: const char kMSEAudioBufferSizeLimit[] = "mse-audio-buffer-size"; On 2016/07/29 21:36:21, alokp wrote: > nit: mse-audio-buffer-size-limit Done. https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:598: if (audio_stream_just_created) { On 2016/07/29 21:36:21, alokp wrote: > can you just move this into "if (!audio_)" block above? why do you need the > audio_stream_just_created boolean? I tried that (see earlier patchsets), but this needs to happen after audio_->UpdateAudioConfig (on line 596), since that's where the SourceBufferStream is actually created when we receive the first init segment. https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:599: std::string audioBufferSwitchStr = On 2016/07/29 21:36:21, alokp wrote: > variable names do not follow chromium style guide. Done. https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:602: unsigned mseAudioBufferSizeLimit = 0; On 2016/07/29 21:36:21, alokp wrote: > variable name Done. https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:622: bool video_stream_just_created = false; On 2016/07/29 21:36:21, alokp wrote: > ditto Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2170303002/diff/1/media/base/media_switches.h... media/base/media_switches.h:70: MEDIA_EXPORT extern const char kMSEAudioBufferSize[]; On 2016/07/29 21:03:25, servolk wrote: > On 2016/07/28 23:02:15, wolenetz wrote: > > On 2016/07/28 21:46:46, servolk wrote: > > > On 2016/07/28 17:55:08, wolenetz wrote: > > > > IIUC, these need to be propogated from browser to renderer process (e.g., > in > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...). > > > > > > > > Have you tested these flags? Can such test be automated? > > > > > > Good point, yeah, I'll add the new flags to that list. I haven't done much > > > testing on this yet, I first wanted to see if you would be ok with making > this > > > configurable via command-line in the first place. > > > As for the automated testing - I guess we could append those switches to the > > > command line, but that will affect all media tests. I'm not sure if that's a > > > good idea. On the other hand we do already have test coverage for the > > > DemuxerStream::SetStreamMemoryLimit being invoked through > > > MediaSourceState::SetMemoryLimits in chunk demuxer, SourceStreamBuffer and > > > pipeline integration tests, so perhaps that's enough and we don't need to > > cover > > > the command line switches specifically. WDYT? > > > > Let's not do this for all media tests. There is a VirtualTestSuite layout test > > functionality which could be used to run a test with a special set of flags > and > > mark it [ Skip ] in TestExpectations to keep it from running on any but the > > intended platform+flag virtual variant. See > > https://codereview.chromium.org/1644763002/diff/160001 for an example. (Would > > probably require a subfolder under http/tests/media/media-source/ to contain > > this specific "test-memory-limit-command-line" test, but seems doable and > > valuable to automate.) > > Ok, yeah, I'll look into adding a virtual test suite-based test. TBH I find the virtual test suites a bit confusing, not sure if I got it right, so I've uploaded the test changes in a separate CL, so that this one is not blocked on the test changes (which might take a few more iterations to get right): https://codereview.chromium.org/2195933002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... File media/filters/media_source_state.cc (right): https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... media/filters/media_source_state.cc:598: if (audio_stream_just_created) { On 2016/07/29 22:14:10, servolk wrote: > On 2016/07/29 21:36:21, alokp wrote: > > can you just move this into "if (!audio_)" block above? why do you need the > > audio_stream_just_created boolean? > > I tried that (see earlier patchsets), but this needs to happen after > audio_->UpdateAudioConfig (on line 596), since that's where the > SourceBufferStream is actually created when we receive the first init segment. OK. Although I would prefer the buffer size limits to be passed in the constructor if it does not make sense to change it at afterwards.
On 2016/07/30 00:08:26, alokp wrote: > https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... > File media/filters/media_source_state.cc (right): > > https://codereview.chromium.org/2170303002/diff/40001/media/filters/media_sou... > media/filters/media_source_state.cc:598: if (audio_stream_just_created) { > On 2016/07/29 22:14:10, servolk wrote: > > On 2016/07/29 21:36:21, alokp wrote: > > > can you just move this into "if (!audio_)" block above? why do you need the > > > audio_stream_just_created boolean? > > > > I tried that (see earlier patchsets), but this needs to happen after > > audio_->UpdateAudioConfig (on line 596), since that's where the > > SourceBufferStream is actually created when we receive the first init segment. > > OK. Although I would prefer the buffer size limits to be passed in the > constructor if it does not make sense to change it at afterwards. Setting buffers at runtime might actually be useful in some scenarios (e.g. in theory we could try reducing MSE buffer sizes at runtime if we detected that the system is running low on memory) and it's being used by unit tests, for example take a look at SourceBufferStreamTest.GarbageCollection_PendingSeek or GarbageCollection_DeleteSeveralRanges. So I think we better leave the current API for greater flexibility.
LGTM % landing tests in https://codereview.chromium.org/2195933002/ (before, or immediately after this CL, as necessary to not break the tree but get coverage ASAP)
On 2016/08/01 20:18:15, wolenetz wrote: > LGTM % landing tests in https://codereview.chromium.org/2195933002/ (before, or > immediately after this CL, as necessary to not break the tree but get coverage > ASAP) Yep, I'm planning to land those tests ASAP
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2170303002/#ps60001 (title: "Adjusted variable and switch names")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make MSE buffer sizes configurable via command line BUG=630342 ========== to ========== Make MSE buffer sizes configurable via command line BUG=630342 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make MSE buffer sizes configurable via command line BUG=630342 ========== to ========== Make MSE buffer sizes configurable via command line BUG=630342 Committed: https://crrev.com/9cc90fd8941a3a2bd5328ff2841ae312b1ffe5b9 Cr-Commit-Position: refs/heads/master@{#409072} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9cc90fd8941a3a2bd5328ff2841ae312b1ffe5b9 Cr-Commit-Position: refs/heads/master@{#409072} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
