media/base/demuxer_stream.h - Issue 1171263004: Allow setting memory limits on media::DemuxerStream

Side by Side Diff: media/base/demuxer_stream.h

Issue 1171263004: Allow setting memory limits on media::DemuxerStream (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: fixed typo Created 5 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #ifndef MEDIA_BASE_DEMUXER_STREAM_H_	5 #ifndef MEDIA_BASE_DEMUXER_STREAM_H_

6 #define MEDIA_BASE_DEMUXER_STREAM_H_	6 #define MEDIA_BASE_DEMUXER_STREAM_H_

7	7

8 #include "base/callback.h"	8 #include "base/callback.h"

9 #include "base/memory/ref_counted.h"	9 #include "base/memory/ref_counted.h"

10 #include "media/base/media_export.h"	10 #include "media/base/media_export.h"

(...skipping 73 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
84 // A DemuxerStream that returns 'true' to this may return the 'kConfigChange'	84 // A DemuxerStream that returns 'true' to this may return the 'kConfigChange'

85 // status from a Read() call. In this case the client is expected to be	85 // status from a Read() call. In this case the client is expected to be

86 // capable of taking appropriate action to handle config changes. Otherwise	86 // capable of taking appropriate action to handle config changes. Otherwise

87 // audio_decoder_config() and video_decoder_config()'s return values are	87 // audio_decoder_config() and video_decoder_config()'s return values are

88 // guaranteed to remain constant, and the client may make optimizations based	88 // guaranteed to remain constant, and the client may make optimizations based

89 // on this.	89 // on this.

90 virtual bool SupportsConfigChanges() = 0;	90 virtual bool SupportsConfigChanges() = 0;

91	91

92 virtual VideoRotation video_rotation() = 0;	92 virtual VideoRotation video_rotation() = 0;

93	93

	94 // Get the data buffer size limit, in bytes.
	ddorwin 2015/06/17 21:02:16 Similar to my comments in https://codereview.chrom Similar to my comments in https://codereview.chromium.org/1182183004/, I don't think "buffer" is correct. servolk 2015/06/17 22:13:48 See the comment in that CL. It's a "buffer" that h Show quoted text On 2015/06/17 21:02:16, ddorwin wrote: > Similar to my comments in https://codereview.chromium.org/1182183004/, I don't > think "buffer" is correct. See the comment in that CL. It's a "buffer" that has the same meaning as in "MSE SourceBuffer", which is different from e.g. media::DecoderBuffer. AFAIK both uses of this word are pretty common. ddorwin 2015/06/20 01:50:26 Those are two examples of qualified uses of "buffe Show quoted text On 2015/06/17 22:13:48, servolk wrote: > On 2015/06/17 21:02:16, ddorwin wrote: > > Similar to my comments in https://codereview.chromium.org/1182183004/, I don't > > think "buffer" is correct. > > See the comment in that CL. It's a "buffer" that has the same meaning as in "MSE > SourceBuffer", which is different from e.g. media::DecoderBuffer. AFAIK both > uses of this word are pretty common. Those are two examples of qualified uses of "buffer"; you have selected one of them as the meaning of an unqualified use of "buffer." SourceBuffer is actually the less common of the two since it's just one mode of the media pipeline. "buffer" is used in the other sense in many more cases in media/. I replied in that CL well.
	95 virtual size_t GetMemoryLimit() const = 0;
	ddorwin 2015/06/17 21:02:16 Is this ever called? I don't see it being used her Is this ever called? I don't see it being used here. SourceBufferStream is the only case that needs it, and it is NOT a DemuxerStream. Maybe it is being called by the new code in https://codereview.chromium.org/1185453004/, but I'm not convinced that design is desirable. Should this really be on a demuxer _stream_ rather than the Demuxer? What behavior would you expect if there were multiple audio streams? Why does DemuxerStream need to implement this? Having to override this with NOTIMPLEMENTED in so many places is a sign of a potential design issue. Perhaps there is another way that would make more sense. Maybe MediaClient. servolk 2015/06/17 22:13:48 It's currently called in the unit tests (see the c Show quoted text On 2015/06/17 21:02:16, ddorwin wrote: > Is this ever called? I don't see it being used here. SourceBufferStream is the > only case that needs it, and it is NOT a DemuxerStream. Maybe it is being called > by the new code in https://codereview.chromium.org/1185453004/, but I'm not > convinced that design is desirable. > > Should this really be on a demuxer _stream_ rather than the Demuxer? What > behavior would you expect if there were multiple audio streams? > > Why does DemuxerStream need to implement this? Having to override this with > NOTIMPLEMENTED in so many places is a sign of a potential design issue. Perhaps > there is another way that would make more sense. Maybe MediaClient. It's currently called in the unit tests (see the changes in media/filters/chunk_demuxer_unittest.cc in this CL). I'm also planning to call this from inside CmaRenderer in Chromecast code for adjust memory limits according to the amount of memory available on the current platform. Coincidentally that's the reason why I'd prefer this API to be on DemuxerStream, instead of media::Demuxer. When a renderer is initialized it is given only media::DemuxerStreamProvider (see media::Renderer::Initialize). So renderers can easily access DemuxerStreams, but not the full media::Demuxer interface. On Chromecast we provide a custom renderer, but the use the rest of the media pipeline as provided by the standard Chromium implementation and we want to be able to use these APIs. Re NOTIMPLEMENTED - take a closer look, most of those NOTIMPLs are in mock/fake objects being used to unit tests. The only two NOTIMPLs that are in actual implementations are in FFmpegDemuxerStream and in MojoDemuxerStreamAdapter. These will need to be implemented eventually (and I have some ideas how to do this for FFmpeg, less clear on how to do this for Mojo). ddorwin 2015/06/20 01:50:26 Is that the amount of memory _currently_ available Show quoted text On 2015/06/17 22:13:48, servolk wrote: > On 2015/06/17 21:02:16, ddorwin wrote: > > Is this ever called? I don't see it being used here. SourceBufferStream is the > > only case that needs it, and it is NOT a DemuxerStream. Maybe it is being > called > > by the new code in https://codereview.chromium.org/1185453004/, but I'm not > > convinced that design is desirable. > > > > Should this really be on a demuxer _stream_ rather than the Demuxer? What > > behavior would you expect if there were multiple audio streams? > > > > Why does DemuxerStream need to implement this? Having to override this with > > NOTIMPLEMENTED in so many places is a sign of a potential design issue. > Perhaps > > there is another way that would make more sense. Maybe MediaClient. > > It's currently called in the unit tests (see the changes in > media/filters/chunk_demuxer_unittest.cc in this CL). I'm also planning to call > this from inside CmaRenderer in Chromecast code for adjust memory limits > according to the amount of memory available on the current platform. Is that the amount of memory _currently_ available or the physical device characteristics? IOW, will the limits change during runtime or just be set per-device? If the former, will they depending on how many DemuxerStreams are active? If the latter, asking MediaClient for an override might be a much simpler solution.
	96

	97 // Set the data buffer size limit, in bytes.

	98 virtual void SetMemoryLimit(size_t memory_limit) = 0;

	99

94 protected:	100 protected:

95 // Only allow concrete implementations to get deleted.	101 // Only allow concrete implementations to get deleted.

96 virtual ~DemuxerStream();	102 virtual ~DemuxerStream();

97 };	103 };

98	104

99 } // namespace media	105 } // namespace media

100	106

101 #endif // MEDIA_BASE_DEMUXER_STREAM_H_	107 #endif // MEDIA_BASE_DEMUXER_STREAM_H_

OLD	NEW

« no previous file with comments | « chromecast/media/cma/test/demuxer_stream_for_test.cc ('k') | media/base/fake_demuxer_stream.h » ('j') | media/filters/ffmpeg_demuxer.cc » ('J')