Move Liveness from DemuxerStreamProvider to DemuxerStream.

Move Liveness from DemuxerStreamProvider to DemuxerStream.

The "Liveness" really is a property of a stream (as in "live stream").
Also this helps us remove a lot of plumbing code for the "liveness" or "low
delay".

Committed: https://crrev.com/2f7e4aca24bb09def39cf2fa0604d95189f9a5fb
Cr-Commit-Position: refs/heads/master@{#304240}

[xhwang, 2014-11-01 06:01:12]

xhwang@chromium.org changed reviewers:
+ wolenetz@chromium.org

[xhwang, 2014-11-01 06:01:13]

wolenetz: PTAL
dalecurtis: FYI

[xhwang, 2014-11-01 06:06:41]

BTW: This doesn't address the issue discussed in issue 420025. But I felt this
is some nice cleanup anyways.

[xhwang, 2014-11-11 17:18:35]

wolenetz: kindly ping!

[wolenetz, 2014-11-11 22:46:30]

On 2014/11/11 17:18:35, xhwang wrote:
> wolenetz: kindly ping!

taking a look now. thanks for your patience :)

[wolenetz, 2014-11-11 23:48:07]

lgtm % nits and double-checks:

https://codereview.chromium.org/692323002/diff/20001/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/692323002/diff/20001/media/base/demuxer_strea...
media/base/demuxer_stream.h:75: virtual Type type() = 0;
nit: adding const seems reasonable to me here (and in overrides).

https://codereview.chromium.org/692323002/diff/20001/media/base/demuxer_strea...
media/base/demuxer_stream.h:78: virtual Liveness liveness();
nit: ditto const

https://codereview.chromium.org/692323002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/692323002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:102:
.WillRepeatedly(Return(Demuxer::LIVENESS_UNKNOWN));
Just a double-check: pipeline no longer should care at all about whether a
demuxer (or the streams produced by a demuxer) has some particular liveness,
right?

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:1645: MEDIA_LOG(log_cb_)
Moving this logic to decoders (or just removing this) is for a later CL as part
of bug 420025 fix, right?

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.h:26: class MEDIA_EXPORT ChunkDemuxerStream : public
DemuxerStream {
nit: In this file, explicitly #include "media/base/demuxer_stream.h"

https://codereview.chromium.org/692323002/diff/20001/media/filters/decoder_st...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/decoder_st...
media/filters/decoder_stream_traits.cc:27:
decoder->Initialize(stream->audio_decoder_config(), status_cb, output_cb);
Another double-check: Audio decoders don't care about "low delay" or liveness,
right?

https://codereview.chromium.org/692323002/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:484: DCHECK_EQ(liveness_, LIVENESS_UNKNOWN);
nit: DCHECK we're on the right thread?

https://codereview.chromium.org/692323002/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:1279: for (const auto& stream : streams_) {  //
|stream| is a ref to a pointer.
nit: DCHECK we're on the right thread?

https://codereview.chromium.org/692323002/diff/20001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:127: low_delay_ = (stream->liveness() ==
DemuxerStream::LIVENESS_LIVE);
nit: This logic is now in multiple places (see also
DecoderStreamTraits<DemuxerStream::VIDEO>::InitializeDecoder(), and inverse in
VideoRendererImplTest::CallInitialize()). Can some of this be simplified (maybe
as a static method in DemuxerStream such as:
static bool is_low_delay(Liveness liveness) { return liveness == LIVENESS_LIVE;
} ) ?

https://codereview.chromium.org/692323002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_demuxer_stream_adapter.cc:52: return
media::DemuxerStream::LIVENESS_UNKNOWN;
nit: this is essentially a duplicate of the default implementation. Remove until
some different version is needed.

[xhwang, 2014-11-14 06:38:05]

https://codereview.chromium.org/692323002/diff/20001/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/692323002/diff/20001/media/base/demuxer_strea...
media/base/demuxer_stream.h:75: virtual Type type() = 0;
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: adding const seems reasonable to me here (and in overrides).

Done.

https://codereview.chromium.org/692323002/diff/20001/media/base/demuxer_strea...
media/base/demuxer_stream.h:78: virtual Liveness liveness();
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: ditto const

Done.

https://codereview.chromium.org/692323002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/692323002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:102:
.WillRepeatedly(Return(Demuxer::LIVENESS_UNKNOWN));
On 2014/11/11 23:48:07, wolenetz wrote:
> Just a double-check: pipeline no longer should care at all about whether a
> demuxer (or the streams produced by a demuxer) has some particular liveness,
> right?

Yes. This is just to suppress warnings.

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:1645: MEDIA_LOG(log_cb_)
On 2014/11/11 23:48:07, wolenetz wrote:
> Moving this logic to decoders (or just removing this) is for a later CL as
part
> of bug 420025 fix, right?

Yes, I'll handle that in a separate CL.

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.h:26: class MEDIA_EXPORT ChunkDemuxerStream : public
DemuxerStream {
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: In this file, explicitly #include "media/base/demuxer_stream.h"

Done.

https://codereview.chromium.org/692323002/diff/20001/media/filters/decoder_st...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/decoder_st...
media/filters/decoder_stream_traits.cc:27:
decoder->Initialize(stream->audio_decoder_config(), status_cb, output_cb);
On 2014/11/11 23:48:07, wolenetz wrote:
> Another double-check: Audio decoders don't care about "low delay" or liveness,
> right?

I think it's either not needed, or not implemented yet.

https://codereview.chromium.org/692323002/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:484: DCHECK_EQ(liveness_, LIVENESS_UNKNOWN);
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: DCHECK we're on the right thread?

Done.

https://codereview.chromium.org/692323002/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:1279: for (const auto& stream : streams_) {  //
|stream| is a ref to a pointer.
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: DCHECK we're on the right thread?

Done.

https://codereview.chromium.org/692323002/diff/20001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:127: low_delay_ = (stream->liveness() ==
DemuxerStream::LIVENESS_LIVE);
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: This logic is now in multiple places (see also
> DecoderStreamTraits<DemuxerStream::VIDEO>::InitializeDecoder(), and inverse in
> VideoRendererImplTest::CallInitialize()). Can some of this be simplified
(maybe
> as a static method in DemuxerStream such as:
> static bool is_low_delay(Liveness liveness) { return liveness ==
LIVENESS_LIVE;
> } )

Hmm, I don't feel

DemuxerStream::is_low_delay(stream->liveness)

is much better than 

stream->liveness() == DemuxerStream::LIVENESS_LIVE

Maybe we don't need liveness at all in DemuxerStream. A boolean "low_delay"
would suffice.

I'll like to iterate on this later.

https://codereview.chromium.org/692323002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_demuxer_stream_adapter.cc:52: return
media::DemuxerStream::LIVENESS_UNKNOWN;
On 2014/11/11 23:48:07, wolenetz wrote:
> nit: this is essentially a duplicate of the default implementation. Remove
until
> some different version is needed.

Done.

[xhwang, 2014-11-14 07:05:56]

rebase only

[xhwang, 2014-11-14 17:46:13]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-11-14 17:47:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692323002/80001

[commit-bot: I haz the power, 2014-11-14 18:37:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-14 18:38:31]

Patchset 5 (id:??) landed as
https://crrev.com/2f7e4aca24bb09def39cf2fa0604d95189f9a5fb
Cr-Commit-Position: refs/heads/master@{#304240}

[wolenetz, 2014-11-14 19:21:58]

https://codereview.chromium.org/692323002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/692323002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:102:
.WillRepeatedly(Return(Demuxer::LIVENESS_UNKNOWN));
On 2014/11/14 06:38:04, xhwang wrote:
> On 2014/11/11 23:48:07, wolenetz wrote:
> > Just a double-check: pipeline no longer should care at all about whether a
> > demuxer (or the streams produced by a demuxer) has some particular liveness,
> > right?
> 
> Yes. This is just to suppress warnings.

Acknowledged.

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:1645: MEDIA_LOG(log_cb_)
On 2014/11/14 06:38:04, xhwang wrote:
> On 2014/11/11 23:48:07, wolenetz wrote:
> > Moving this logic to decoders (or just removing this) is for a later CL as
> part
> > of bug 420025 fix, right?
> 
> Yes, I'll handle that in a separate CL.

Acknowledged.

https://codereview.chromium.org/692323002/diff/20001/media/filters/decoder_st...
File media/filters/decoder_stream_traits.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/decoder_st...
media/filters/decoder_stream_traits.cc:27:
decoder->Initialize(stream->audio_decoder_config(), status_cb, output_cb);
On 2014/11/14 06:38:04, xhwang wrote:
> On 2014/11/11 23:48:07, wolenetz wrote:
> > Another double-check: Audio decoders don't care about "low delay" or
liveness,
> > right?
> 
> I think it's either not needed, or not implemented yet.

Acknowledged.

https://codereview.chromium.org/692323002/diff/20001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/692323002/diff/20001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:127: low_delay_ = (stream->liveness() ==
DemuxerStream::LIVENESS_LIVE);
On 2014/11/14 06:38:04, xhwang wrote:
> On 2014/11/11 23:48:07, wolenetz wrote:
> > nit: This logic is now in multiple places (see also
> > DecoderStreamTraits<DemuxerStream::VIDEO>::InitializeDecoder(), and inverse
in
> > VideoRendererImplTest::CallInitialize()). Can some of this be simplified
> (maybe
> > as a static method in DemuxerStream such as:
> > static bool is_low_delay(Liveness liveness) { return liveness ==
> LIVENESS_LIVE;
> > } )
> 
> Hmm, I don't feel
> 
> DemuxerStream::is_low_delay(stream->liveness)
> 
> is much better than 
> 
> stream->liveness() == DemuxerStream::LIVENESS_LIVE
> 
> Maybe we don't need liveness at all in DemuxerStream. A boolean "low_delay"
> would suffice.
> 
> I'll like to iterate on this later.

Acknowledged.