Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
164233005:
Cap the memory usage in FFMpegDemuxer. (Closed)
|
Created:
6 years, 10 months ago by damienv1 Modified:
6 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Ami GONE FROM CHROMIUM Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCap the memory usage in FFMpegDemuxer.
Abort the playback if a specific stream triggers some high memory usage.
BUG=343304
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252259
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Address comments from patch set #2. #
Total comments: 2
Patch Set 4 : #
Total comments: 5
Patch Set 5 : Address comments from patch set #4. #Patch Set 6 : Forgot to address a nit from patch set #3. #Patch Set 7 : Revert to IsMaxMemoryUsage api style in FFMpegDemuxer. #
Total comments: 7
Patch Set 8 : Use size_t instead of int. #
Total comments: 4
Patch Set 9 : Address comments from patch set #8. #
Messages
Total messages: 28 (0 generated)
PTAL.
are there UMA stats that say this is a problem worth solving? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
* From UMA, it's hard to tell how much memory is used specifically by FFMpegDemuxer: we have only the global memory usage of the renderer process. * I agree that videos from the Web don't probably belong to the category of videos that might trigger the issues I mentioned in the bug. So Chrome desktop (win/linux/mac) is not likely to be impacted. However, all personal videos loaded in the browser might be subject to this issue. ChromeOS / Chromecast are the most likely to be impacted. * I think it's also a question of tradeoff here: there is a potential OOM triggered by feeding special streams. Do we want to avoid that situation ? To me, the code change here is small enough and does not involve design change. * I could add a UMA log to monitor the memory used by FFMpegDemuxer if you think it is worth it. https://codereview.chromium.org/164233005/diff/140001/media/base/decoder_buff... File media/base/decoder_buffer_queue.cc (left): https://codereview.chromium.org/164233005/diff/140001/media/base/decoder_buff... media/base/decoder_buffer_queue.cc:12: #include <limits>
The "tradeoff" is not between (possibly infrequent) OOMs and the LOC delta of this CL. The tradeoff is between OOM and failing to play media that today plays just fine without OOMing the renderer. In order for this to be a good tradeoff you must find the "sweet spot" for the max parameters: it can't be too high b/c then it provides no value over having no limit, and it can't be too low b/c then you end up refusing to play media that today would play fine, if a bit expensively in RAM. So I'm curious how you justify the current values you chose for maximums. Even if we have good threshold values (that separate legit media from media that OOM the renderer), how big is the benefit to the use-case of personal media? IOW is it really more useful to see a black video canvas (demux/decode error) than a sad tab? In neither case can the user actually play the media they wanted to play... On Fri, Feb 14, 2014 at 9:23 AM, <damienv@chromium.org> wrote: > * From UMA, it's hard to tell how much memory is used specifically by > FFMpegDemuxer: we have only the global memory usage of the renderer > process. > > * I agree that videos from the Web don't probably belong to the category of > videos that might trigger the issues I mentioned in the bug. So Chrome > desktop > (win/linux/mac) is not likely to be impacted. > However, all personal videos loaded in the browser might be subject to this > issue. ChromeOS / Chromecast are the most likely to be impacted. > > * I think it's also a question of tradeoff here: there is a potential OOM > triggered by feeding special streams. Do we want to avoid that situation ? > To > me, the code change here is small enough and does not involve design > change. > > * I could add a UMA log to monitor the memory used by FFMpegDemuxer if you > think > it is worth it. > > > https://codereview.chromium.org/164233005/diff/140001/ > media/base/decoder_buffer_queue.cc > File media/base/decoder_buffer_queue.cc (left): > > https://codereview.chromium.org/164233005/diff/140001/ > media/base/decoder_buffer_queue.cc#oldcode12 > media/base/decoder_buffer_queue.cc:12: > #include <limits> > > https://codereview.chromium.org/164233005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
1) I now understand your point regarding the threshold. So far, I use the same values as what is defined in media/filters/source_buffer_stream.cc (kDefaultAudioMemoryLimit & kDefaultVideoMemoryLimit). I know it is not exactly the same use but that gives a good approximation of what is acceptable. 2) Regarding your point between a sad tab and a black video canavas, there are some differences. We can image a JS app that loads a playlist of personal videos: in this case, the web app will crash in one case while it could play the next video of the playlist in the other case.
I had misread your video limit as 15M instead of 150M. I don't have a strong feeling about this tradeoff, and defer to acolwell@as master of demuxers. On Fri, Feb 14, 2014 at 9:52 AM, <damienv@chromium.org> wrote: > 1) I now understand your point regarding the threshold. > So far, I use the same values as what is defined in > media/filters/source_buffer_stream.cc (kDefaultAudioMemoryLimit & > kDefaultVideoMemoryLimit). I know it is not exactly the same use but that > gives > a good approximation of what is acceptable. > > 2) Regarding your point between a sad tab and a black video canavas, there > are > some differences. We can image a JS app that loads a playlist of personal > videos: in this case, the web app will crash in one case while it could > play the > next video of the playlist in the other case. > > https://codereview.chromium.org/164233005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+acolwell to R line, move fischman to CC.
I wonder if we should use time-based limits instead of memory based ones. Is the problem that we are running out of memory or is it that badly muxed files allow minutes of data to be buffered in one stream before data becomes available in another stream. If we still deem that a memory limit is the way to go, why not just use a single limit and just prevent the sum of the stream's memory usage from going over that limit. https://codereview.chromium.org/164233005/diff/140001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/140001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:934: default: nit: Please don't use default here. Explicitly indicate the ones we are ignoring so we get a compiler error if a new type gets added.
Thanks for the feedback: 1) I think what matters here is really the memory usage so the memory usage should be one of the criteria. However, we could introduce two limits if needed: one in terms of memory usage and one in terms of buffer duration. Please note that badly muxed streams are not the only use case. A correctly muxed sparse video streams could trigger this issue as well. 2) I agree that if we stick to a memory cap, then the criteria should not be stream based but should rather be a criteria on the overall memory usage among demuxer streams. I will update the CL to take the overall FFMpegDemuxer memory usage into account.
https://codereview.chromium.org/164233005/diff/140001/media/base/decoder_buff... File media/base/decoder_buffer_queue.cc (left): https://codereview.chromium.org/164233005/diff/140001/media/base/decoder_buff... media/base/decoder_buffer_queue.cc:12: On 2014/02/14 17:23:33, damienv1 wrote: > #include <limits> Done. https://codereview.chromium.org/164233005/diff/140001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/140001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:934: default: On 2014/02/18 22:16:20, acolwell wrote: > nit: Please don't use default here. Explicitly indicate the ones we are ignoring > so we get a compiler error if a new type gets added. I changed the code so that the contribution of every DemuxerStream is included. + The criteria is now on the global FFMpegDemuxer memory usage.
lgtm % nit https://codereview.chromium.org/164233005/diff/260001/media/base/decoder_buff... File media/base/decoder_buffer_queue_unittest.cc (right): https://codereview.chromium.org/164233005/diff/260001/media/base/decoder_buff... media/base/decoder_buffer_queue_unittest.cc:138: TEST(DecoderBufferQueueTest, Size) { nit: s/Size/DataSize/ https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:930: memory_usage += (*iter)->MemoryUsage(); nit: I'm a little more worried about overflow here. How about we track memory_free instead of memory_usage and early return if ((*iter)->MemoryUsage() > memory_free). I think that might be a little safer.
https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:918: bool FFmpegDemuxer::IsMaxMemoryReached() { Better design: have a MemoryUsage function (just like we have a MemoryUsage in FFMpegDemuxerStream), do not compare against the memory limit here.
https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:918: bool FFmpegDemuxer::IsMaxMemoryReached() { On 2014/02/19 00:15:09, damienv1 wrote: > Better design: have a MemoryUsage function (just like we have a MemoryUsage in > FFMpegDemuxerStream), do not compare against the memory limit here. Done. https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:923: StreamVector::iterator iter; To be removed (already in the for loop). https://codereview.chromium.org/164233005/diff/330001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:930: memory_usage += (*iter)->MemoryUsage(); On 2014/02/18 23:57:49, acolwell wrote: > nit: I'm a little more worried about overflow here. How about we track > memory_free instead of memory_usage and early return if ((*iter)->MemoryUsage() > > memory_free). I think that might be a little safer. I was a bit worried as well. So I agree and I am going to add a condition instead of a DCHECK.
https://codereview.chromium.org/164233005/diff/260001/media/base/decoder_buff... File media/base/decoder_buffer_queue_unittest.cc (right): https://codereview.chromium.org/164233005/diff/260001/media/base/decoder_buff... media/base/decoder_buffer_queue_unittest.cc:138: TEST(DecoderBufferQueueTest, Size) { On 2014/02/18 23:57:48, acolwell wrote: > nit: s/Size/DataSize/ Done.
Please let me know if you have additional feedback. Otherwise, I plan to merge the change by EOD. Thanks.
lgtm
I defer to acolwell@'s judgement on the merits of this, but I have a couple of style nits. https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... File media/base/decoder_buffer_queue.cc (right): https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... media/base/decoder_buffer_queue.cc:27: DCHECK_LE(buffer->data_size(), std::numeric_limits<int>::max() - data_size_); This should be a CHECK(), you may need to CHECK(data_size() > 0) too. https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... File media/base/decoder_buffer_queue.h (right): https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... media/base/decoder_buffer_queue.h:55: int data_size() const { return data_size_; } I'd just make this a size_t to minimize the CHECKs you need. The style guide now recommends size_t for "size" types. https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... File media/base/decoder_buffer_queue_unittest.cc (right): https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... media/base/decoder_buffer_queue_unittest.cc:21: static scoped_refptr<DecoderBuffer> CreateBuffer(int timestamp, int size = 0) { We don't use defaults in Chromium code. https://codereview.chromium.org/164233005/diff/520001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/164233005/diff/520001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:98: int MemoryUsage() const; size_t again.
Dale, I can do the change to size_t. However for consistency, I preferred using "int". Please let me know if you think that mixing types still makes sense or you think that it should be part of another CL (that's my feeling). https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... File media/base/decoder_buffer_queue.h (right): https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... media/base/decoder_buffer_queue.h:55: int data_size() const { return data_size_; } On 2014/02/19 18:59:25, DaleCurtis wrote: > I'd just make this a size_t to minimize the CHECKs you need. The style guide now > recommends size_t for "size" types. I agree that size_t should have been used in a first place. However, I used "int" for consistency with DecoderBuffer::data_size which currently returns "int". Does it make sense to mix both data types ? My first feeling was to use the same type and if we want to change to the suggested size_t type, then we would change both DecoderBuffer and DecoderBufferQueue all together.
My preference is for your new code to be size_t w/ checked cast of DecoderBuffer's size which can be cleaned up in a follow up CL.
https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... File media/base/decoder_buffer_queue_unittest.cc (right): https://codereview.chromium.org/164233005/diff/520001/media/base/decoder_buff... media/base/decoder_buffer_queue_unittest.cc:21: static scoped_refptr<DecoderBuffer> CreateBuffer(int timestamp, int size = 0) { On 2014/02/19 18:59:25, DaleCurtis wrote: > We don't use defaults in Chromium code. Done. https://codereview.chromium.org/164233005/diff/520001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/164233005/diff/520001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:98: int MemoryUsage() const; On 2014/02/19 18:59:25, DaleCurtis wrote: > size_t again. Done.
lgtm https://codereview.chromium.org/164233005/diff/720001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/720001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:326: size_t FFmpegDemuxerStream::MemoryUsage() const { You can declare this inline as memory_usage() if you prefer. https://codereview.chromium.org/164233005/diff/720001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:361: static const size_t kDemuxerMemoryLimit = 150 * 1024 * 1024; Since this is only used in one spot, just stick it down below inside the function using it.
https://codereview.chromium.org/164233005/diff/720001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/164233005/diff/720001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:326: size_t FFmpegDemuxerStream::MemoryUsage() const { On 2014/02/19 20:56:21, DaleCurtis wrote: > You can declare this inline as memory_usage() if you prefer. Prefer to keep the current approach (similar to HasAvailableCapacity). https://codereview.chromium.org/164233005/diff/720001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:361: static const size_t kDemuxerMemoryLimit = 150 * 1024 * 1024; On 2014/02/19 20:56:21, DaleCurtis wrote: > Since this is only used in one spot, just stick it down below inside the > function using it. Done.
The CQ bit was checked by damienv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/164233005/810001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/164233005/810001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/164233005/810001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/164233005/810001
Message was sent while issue was closed.
Change committed as 252259 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
