Add live mode detection in WebM MediaSource parser.

Add live mode detection in WebM MediaSource parser.

The live mode is indicated by presense of DateUTC element and unknown
segment duration and size.

BUG=338529
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266452

[scherkus (not reviewing), 2014-04-10 18:17:52]

overall looks fine

can we turn your test site into an end to end test?

you can either go the C++ route and make this part of
pipeline_integration_test.cc, or a higher-level test such as a
content_browsertest that spins up MSE and verifies it gets the low-latency
output:
content/browser/media/media_source_browsertest.cc

[Sergey Ulanov, 2014-04-10 19:40:30]

On 2014/04/10 18:17:52, scherkus wrote:
> overall looks fine
> 
> can we turn your test site into an end to end test?
> 
> you can either go the C++ route and make this part of
> pipeline_integration_test.cc, or a higher-level test such as a
> content_browsertest that spins up MSE and verifies it gets the low-latency
> output:
> content/browser/media/media_source_browsertest.cc

Yes, I do plan to add a test, but I think it has to be a separate CL - this CL
is not enough to fix 338529. There are two other things that need to be fixed:
  - GpuVideoDecoder still delays frames.
  - I'd like to do something about the preroll delay. One option is to disable
preroll completely for live streams. Another option is to use SeekPreRoll
element in WebM to specify desired preroll length.

[scherkus (not reviewing), 2014-04-10 21:11:48]

few sanity checks 'n nits -- I don't mind whether it's low-delay or live mode,
but we should try to be consistent with our terminology


also please update the CL description to mention that FFmpegVideoDecoder is
taking advantage of this information to disable parallel frame decoding to
decrease decoder delay

https://codereview.chromium.org/231283005/diff/10001/media/base/video_decoder...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/231283005/diff/10001/media/base/video_decoder...
media/base/video_decoder_config.h:140: // Returns true if the decoder should try
to minimize decoding latency.
compare "live_mode()" to the documentation that discusses decoding latency

https://codereview.chromium.org/231283005/diff/10001/media/ffmpeg/ffmpeg_comm...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/ffmpeg/ffmpeg_comm...
media/ffmpeg/ffmpeg_common.cc:429: false,  // Low-delay mode is off.
compare "live mode" parameter vs. usage of "low delay mode"

https://codereview.chromium.org/231283005/diff/10001/media/filters/decrypting...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/filters/decrypting...
media/filters/decrypting_demuxer_stream.cc:389: false,  // Live mode is off.
ditto, except now it's live mode

https://codereview.chromium.org/231283005/diff/10001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:347: codec_context_->thread_type =
hmm... did we still want threaded slicing or to disable threading altogether?

https://codereview.chromium.org/231283005/diff/10001/media/formats/webm/webm_...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/formats/webm/webm_...
media/formats/webm/webm_tracks_parser_unittest.cc:132:
scoped_ptr<WebMTracksParser> parser(new WebMTracksParser(LogCB(), true, false));
80 chars

[Sergey Ulanov, 2014-04-10 23:10:06]

https://codereview.chromium.org/231283005/diff/10001/media/base/video_decoder...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/231283005/diff/10001/media/base/video_decoder...
media/base/video_decoder_config.h:140: // Returns true if the decoder should try
to minimize decoding latency.
On 2014/04/10 21:11:49, scherkus wrote:
> compare "live_mode()" to the documentation that discusses decoding latency

Done.

https://codereview.chromium.org/231283005/diff/10001/media/ffmpeg/ffmpeg_comm...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/ffmpeg/ffmpeg_comm...
media/ffmpeg/ffmpeg_common.cc:429: false,  // Low-delay mode is off.
On 2014/04/10 21:11:49, scherkus wrote:
> compare "live mode" parameter vs. usage of "low delay mode"

Done.

https://codereview.chromium.org/231283005/diff/10001/media/filters/decrypting...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/filters/decrypting...
media/filters/decrypting_demuxer_stream.cc:389: false,  // Live mode is off.
On 2014/04/10 21:11:49, scherkus wrote:
> ditto, except now it's live mode

Done.

https://codereview.chromium.org/231283005/diff/10001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.cc:347: codec_context_->thread_type =
On 2014/04/10 21:11:49, scherkus wrote:
> hmm... did we still want threaded slicing or to disable threading altogether?

I think slicing may be useful for live streams to minimize latency introduced by
the decoder.

https://codereview.chromium.org/231283005/diff/10001/media/formats/webm/webm_...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/231283005/diff/10001/media/formats/webm/webm_...
media/formats/webm/webm_tracks_parser_unittest.cc:132:
scoped_ptr<WebMTracksParser> parser(new WebMTracksParser(LogCB(), true, false));
On 2014/04/10 21:11:49, scherkus wrote:
> 80 chars

Done.

[acolwell GONE FROM CHROMIUM, 2014-04-11 15:04:09]

I haven't looked at all of this code yet, but I'd like to at least put the
demuxer & parser changes on hold for a little bit. On the plane back from Vegas
I implemented support for the DateUTC field in both demuxers to support
HTMLMediaElement::getStartDate(). I'd like this change to build on top of that.
I plan on cleaning up and sending out my CL for review today.

[Sergey Ulanov, 2014-04-21 19:23:24]

PTAL. Merged with the trunk that contains your change for DateUTC

On 2014/04/11 15:04:09, acolwell wrote:
> I haven't looked at all of this code yet, but I'd like to at least put the
> demuxer & parser changes on hold for a little bit. On the plane back from
Vegas
> I implemented support for the DateUTC field in both demuxers to support
> HTMLMediaElement::getStartDate(). I'd like this change to build on top of
that.
> I plan on cleaning up and sending out my CL for review today.

[Sergey Ulanov, 2014-04-23 02:50:14]

acolwell, scherkus: ping

[acolwell GONE FROM CHROMIUM, 2014-04-23 16:06:02]

Putting "live mode" in VideoDecoderConfig doesn't seem right to me. The
"live"-ness is a presentation-level property much like duration, start time, and
timeline offset are. I think it should be exposed by the demuxer in a similar
manner as start time or timeline offset. The pipeline can then pass this
information to the VideoRenderer & VideoDecoder where necessary.

https://codereview.chromium.org/231283005/diff/70001/media/formats/webm/webm_...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/231283005/diff/70001/media/formats/webm/webm_...
media/formats/webm/webm_stream_parser.cc:21: static const int64
kUnknownElementSize = 0xffffffffffffff;
nit: Please remove and use the kWebMUnknownSize constant defined in
webm_constants.h

[Sergey Ulanov, 2014-04-23 21:08:37]

On 2014/04/23 16:06:02, acolwell wrote:
> Putting "live mode" in VideoDecoderConfig doesn't seem right to me. The
> "live"-ness is a presentation-level property much like duration, start time,
and
> timeline offset are. I think it should be exposed by the demuxer in a similar
> manner as start time or timeline offset. The pipeline can then pass this
> information to the VideoRenderer & VideoDecoder where necessary.

This parameter needs to be passed to the ffmpeg decoder and currently there is
no other way to pass configuration parameters to decoders other than
VideoDecoderConfig. So even if we store and expose it in the demuxer, it still
needs to be passed to VideoDecoderConfig. We could have a separate argument in
VideoDecoder::Initialize(), but I don't think that would be better than adding
it in VideoDecoderConfig. WDYT? Is there any other way to pass this parameter to
the decoder?

[Sergey Ulanov, 2014-04-24 02:55:14]

PTAL.
now live_mode() is exposed in Demuxer, and I renamed the parameter in
VideoDecoderConfig to |low_delay| as that makes more sense at the decoder layer.
Also let me know what you think about my suggestions in the comments - these
would make this change much smaller and simpler.

https://codereview.chromium.org/231283005/diff/90001/media/base/stream_parser.h
File media/base/stream_parser.h (right):

https://codereview.chromium.org/231283005/diff/90001/media/base/stream_parser...
media/base/stream_parser.h:66: bool live_mode)> InitCB;
IMO it's better to add a struct (e.g. StreamParameters or something like that)
to make it easier to add new parameters here. let me know what you think - I'll
do it in a separate change if you agree.

https://codereview.chromium.org/231283005/diff/90001/media/base/video_decoder...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/231283005/diff/90001/media/base/video_decoder...
media/base/video_decoder_config.h:141: bool low_delay() const;
Can we make individual fields mutable? (i.e. add set_low_delay() here) It would
make this change much smaller (and any other change that adds new fields in the
config).

[acolwell GONE FROM CHROMIUM, 2014-04-24 16:59:35]

I really think we should split this up into a few CLs.
1. Create a StreamParserParams struct.
2. A CL that adds the live_mode to the StreamParserParams struct, adds the
parsing logic, and exposes the live_mode() through the Demuxer interface.
3. A CL where the pipeline initializes the video renderer & related classes with
the live_mode() info so the decoder can be configured in the low delay mode. 

I want to keep the low delay mode out of VideoDecoderConfig because it is more
of a "live playback policy" decision and not really information that describes
the video stream. This also sets the stage for audio related changes if we ever
need a live mode for that as well.

https://codereview.chromium.org/231283005/diff/90001/media/base/stream_parser.h
File media/base/stream_parser.h (right):

https://codereview.chromium.org/231283005/diff/90001/media/base/stream_parser...
media/base/stream_parser.h:66: bool live_mode)> InitCB;
On 2014/04/24 02:55:14, Sergey Ulanov wrote:
> IMO it's better to add a struct (e.g. StreamParameters or something like that)
> to make it easier to add new parameters here. let me know what you think -
I'll
> do it in a separate change if you agree.

Yeah. We should probably have a struct at this point.

[Sergey Ulanov, 2014-04-24 18:19:12]

On Thu, Apr 24, 2014 at 9:59 AM, <acolwell@chromium.org> wrote:

> I really think we should split this up into a few CLs.
> 1. Create a StreamParserParams struct.
>

I think it's better to call it StreamParams - they are parameters of the
stream, not the parser.



> 2. A CL that adds the live_mode to the StreamParserParams struct, adds the
> parsing logic, and exposes the live_mode() through the Demuxer interface.
> 3. A CL where the pipeline initializes the video renderer & related
> classes with
> the live_mode() info so the decoder can be configured in the low delay
> mode.
>

SGTM


>
> I want to keep the low delay mode out of VideoDecoderConfig because it is
> more
> of a "live playback policy" decision and not really information that
> describes
> the video stream. This also sets the stage for audio related changes if we
> ever
> need a live mode for that as well.


How do you suggest passing this flag to the decoder without adding it to
VideoDecoderConfig?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[acolwell GONE FROM CHROMIUM, 2014-04-24 19:16:41]

On 2014/04/24 18:19:12, Sergey Ulanov wrote:
> On Thu, Apr 24, 2014 at 9:59 AM, <mailto:acolwell@chromium.org> wrote:
> 
> > I really think we should split this up into a few CLs.
> > 1. Create a StreamParserParams struct.
> >
> 
> I think it's better to call it StreamParams - they are parameters of the
> stream, not the parser.

Hmm. Unfortunately "stream" is somewhat overloaded of a term. The "Stream" in
StreamParser is supposed to refer to the "byte stream" that is passed in. I
worry that StreamParams may cause people to think of Stream in the sense of a
track here, which it definitely is not. How about Metadata instead since this
stuff is basically presentation metadata?

> 
> 
> 
> > 2. A CL that adds the live_mode to the StreamParserParams struct, adds the
> > parsing logic, and exposes the live_mode() through the Demuxer interface.
> > 3. A CL where the pipeline initializes the video renderer & related
> > classes with
> > the live_mode() info so the decoder can be configured in the low delay
> > mode.
> >
> 
> SGTM
> 
> 
> >
> > I want to keep the low delay mode out of VideoDecoderConfig because it is
> > more
> > of a "live playback policy" decision and not really information that
> > describes
> > the video stream. This also sets the stage for audio related changes if we
> > ever
> > need a live mode for that as well.
> 
> 
> How do you suggest passing this flag to the decoder without adding it to
> VideoDecoderConfig?

I think it should go through the video renderer's Initialize() method and make
it's way to the decoder that way.

[Sergey Ulanov, 2014-04-24 19:29:47]

On 2014/04/24 19:16:41, acolwell wrote:
> On 2014/04/24 18:19:12, Sergey Ulanov wrote:
> > On Thu, Apr 24, 2014 at 9:59 AM, <mailto:acolwell@chromium.org> wrote:
> > 
> > > I really think we should split this up into a few CLs.
> > > 1. Create a StreamParserParams struct.
> > >
> > 
> > I think it's better to call it StreamParams - they are parameters of the
> > stream, not the parser.
> 
> Hmm. Unfortunately "stream" is somewhat overloaded of a term. The "Stream" in
> StreamParser is supposed to refer to the "byte stream" that is passed in. I
> worry that StreamParams may cause people to think of Stream in the sense of a
> track here, which it definitely is not. How about Metadata instead since this
> stuff is basically presentation metadata?
> 

I've just sent you a CL where StreamParameters is nested in StreamParser - I
think it's not 
ambiguous there, but let me know if you'd like to rename it.


> > How do you suggest passing this flag to the decoder without adding it to
> > VideoDecoderConfig?
> 
> I think it should go through the video renderer's Initialize() method and make
> it's way to the decoder that way.

So how should we pass the value to the decoder? Add another argument in
VideoDecoder::Initialize()?

[acolwell GONE FROM CHROMIUM, 2014-04-24 19:37:35]

On 2014/04/24 19:29:47, Sergey Ulanov wrote:
> On 2014/04/24 19:16:41, acolwell wrote:
> > On 2014/04/24 18:19:12, Sergey Ulanov wrote:
> > > On Thu, Apr 24, 2014 at 9:59 AM, <mailto:acolwell@chromium.org> wrote:
> > > 
> > > > I really think we should split this up into a few CLs.
> > > > 1. Create a StreamParserParams struct.
> > > >
> > > 
> > > I think it's better to call it StreamParams - they are parameters of the
> > > stream, not the parser.
> > 
> > Hmm. Unfortunately "stream" is somewhat overloaded of a term. The "Stream"
in
> > StreamParser is supposed to refer to the "byte stream" that is passed in. I
> > worry that StreamParams may cause people to think of Stream in the sense of
a
> > track here, which it definitely is not. How about Metadata instead since
this
> > stuff is basically presentation metadata?
> > 
> 
> I've just sent you a CL where StreamParameters is nested in StreamParser - I
> think it's not 
> ambiguous there, but let me know if you'd like to rename it.
ok. I'll take a look.

> 
> 
> > > How do you suggest passing this flag to the decoder without adding it to
> > > VideoDecoderConfig?
> > 
> > I think it should go through the video renderer's Initialize() method and
make
> > it's way to the decoder that way.
> 
> So how should we pass the value to the decoder? Add another argument in
> VideoDecoder::Initialize()?

yes.

[Sergey Ulanov, 2014-04-24 20:05:37]

PTAL: rebased this change on top of https://codereview.chromium.org/254473006
and removed decoder changes.

[acolwell GONE FROM CHROMIUM, 2014-04-24 21:14:15]

looks good. Just a few minor comments.

https://codereview.chromium.org/231283005/diff/150001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/231283005/diff/150001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1517: live_mode_ |= params.live_mode;
I feel like we should enforce an all the same or error like timeline_offset_
does. Perhaps use an enum { NotSet, Live, NotLive} for live_mode_. and signal an
error if we get inconsistent values for params.live_mode.

https://codereview.chromium.org/231283005/diff/150001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/231283005/diff/150001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:689: max_duration = kInfiniteDuration();
We should probably reflect this assumption via IsLiveMode() as well.

[Sergey Ulanov, 2014-04-24 23:17:44]

https://codereview.chromium.org/231283005/diff/150001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/231283005/diff/150001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1517: live_mode_ |= params.live_mode;
On 2014/04/24 21:14:16, acolwell wrote:
> I feel like we should enforce an all the same or error like timeline_offset_
> does. Perhaps use an enum { NotSet, Live, NotLive} for live_mode_. and signal
an
> error if we get inconsistent values for params.live_mode.

Done.

https://codereview.chromium.org/231283005/diff/150001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/231283005/diff/150001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:689: max_duration = kInfiniteDuration();
On 2014/04/24 21:14:16, acolwell wrote:
> We should probably reflect this assumption via IsLiveMode() as well.

Done. For consistency with the WebM parser it checks both timeline_offset_ and
max_duration to detect live mode.

[acolwell GONE FROM CHROMIUM, 2014-04-25 23:30:51]

lgtm % nits

https://codereview.chromium.org/231283005/diff/210001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/231283005/diff/210001/media/base/demuxer.h#ne...
media/base/demuxer.h:101: // Returns liveness of the stream, i.e. whether it
recorded or live.
nit: s/it/it is/

https://codereview.chromium.org/231283005/diff/210001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/231283005/diff/210001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:512: return LIVENESS_UNKNOWN;
nit: Shouldn't this return liveness_?

[Sergey Ulanov, 2014-04-26 01:51:03]

https://codereview.chromium.org/231283005/diff/210001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/231283005/diff/210001/media/base/demuxer.h#ne...
media/base/demuxer.h:101: // Returns liveness of the stream, i.e. whether it
recorded or live.
On 2014/04/25 23:30:52, acolwell wrote:
> nit: s/it/it is/

Done.

https://codereview.chromium.org/231283005/diff/210001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/231283005/diff/210001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:512: return LIVENESS_UNKNOWN;
On 2014/04/25 23:30:52, acolwell wrote:
> nit: Shouldn't this return liveness_?

Yes, it should :)

[Sergey Ulanov, 2014-04-26 01:51:19]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-04-26 02:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/231283005/230001

[commit-bot: I haz the power, 2014-04-26 03:06:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 03:06:20]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[Sergey Ulanov, 2014-04-27 07:55:54]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-04-27 07:56:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/231283005/230001

[commit-bot: I haz the power, 2014-04-28 06:51:04]

Change committed as 266452