1159027 - Create new MP4Demuxer, a MediaDataDemuxer object

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This object would follow the new MediaDataDemuxer API as described in bug 1154164

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Add MP4Metadata method to retrieve init segment byte range

Add method to retrieve position of the MP4 moov in the stream

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Extend BufferStream to work with external buffer array

Make BufferStream able to work with a MediaLargeByteBuffer fallible array

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add MP4Demuxer object

Add MP4Demuxer object

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Implement SkipToNextRandomAccessPoint for MP4

Implement SkipToNextRandomAccessPoint for MP4Demuxer

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8602451 [details] [diff] [review]
Part3. Add MP4Demuxer object

Will merge p3 and p4 together

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add MP4Demuxer object

MP4Demuxer object.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Include ftyp box when parsing init segment

An init segment is a ftyp *and* a moov as per spec. Otherwise it may cause stagefright to fail parsing the moov

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8603170 [details] [diff] [review]
Part4. Include ftyp box when parsing init segment

Review of attachment 8603170 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/MoofParser.cpp
@@ +140,5 @@
>    }
> +  if (!ftyp.Length() || !moov.Length()) {
> +    return false;
> +  }
> +  mInitRange = MediaByteRange(ftyp.mStart, moov.mEnd);

Should be:
  mInitRange = ftype.Extents(moov);

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Ensure we have up to date index when calculating offsets

Implement MP4Demuxer::NotifyDataArrived. We'll need it afterall

Comment 10

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8603120 [details] [diff] [review]
Part3. Add MP4Demuxer object

Review of attachment 8603120 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, very nice.

I would like to know whether we still need to resolve the InitPromise with WAITING_FOR_DATA, or whether we can just delay resolving it.

::: dom/media/fmp4/MP4Demuxer.cpp
@@ +31,5 @@
> +
> +  // Check that we have enough data to read the metadata.
> +  MediaByteRange br = mp4_demuxer::MP4Metadata::MetadataRange(stream);
> +  if (br.IsNull()) {
> +    return InitPromise::CreateAndReject(DemuxerFailureReason::WAITING_FOR_DATA, __func__);

Can't we just return an unresolved promise here and retry when NotifyDataArrived is called? Or does MSE or the reader have to handle the waiting for data state?

Just trying to figure out if we can make the caller cleaner here.

@@ +172,5 @@
> +  MonitorAutoLock mon(mMonitor);
> +  mIterator->Seek(seekTime);
> +
> +  // Check what time we actually seeked to.
> +  mQueuedSample = mIterator->GetNext();

Would it make sense to add a PeekNext() function to the iterator so that we don't have to queue the sample?

@@ +237,5 @@
> +      aSamples.LastElement()->mTime >= mNextKeyframeTime.value().ToMicroseconds()) {
> +    mNextKeyframeTime.reset();
> +    mp4_demuxer::Microseconds frameTime = mIterator->GetNextKeyframeTime();
> +    if (frameTime != -1) {
> +      mNextKeyframeTime.emplace(

Shouldn't media::TimeUnit::emplace(TimeUnit) not need you to convert from a TimeUnit to a TimeUnit? i.e. I'm surprised you need to call FromMicroseconds on a TimeUnit when assigning a TimeUnit to a TimeUnit.

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #10)

> Overall, very nice.
> 
> I would like to know whether we still need to resolve the InitPromise with
> WAITING_FOR_DATA, or whether we can just delay resolving it.

I answered that question (or so I hope) in bug 1154164.

It's also dependent on the ReadMediaData to also return a WAITING_FOR_DATA if it can't do it all.

We can probably change this in the future, but at present the MDSM is expecting the ReadMetadata call to return a WAITING_FOR_DATA and that will cause it to retry later.

The MediaFormatDecoder::AsyncReadMetadata is heavily dependent on the init of the demuxer to resolve the promise immediately.

It's the same deal with RequestAudio/VideoData. We return a WAITING_FOR_DATA promise, while Seek on the other hand is expected to only resolve the promise once it has all the data.

It make things inconsistent and I think we can also address this at a later stage.

> Can't we just return an unresolved promise here and retry when
> NotifyDataArrived is called? Or does MSE or the reader have to handle the
> waiting for data state?
> 
> Just trying to figure out if we can make the caller cleaner here.

I long debated with myself on what was best here... The current MSE implementation does require to know that we don't have enough data. And to incrementally use MP4Demuxer / MediaFormatDecoder with the existing MSE code it has to be done that way.

> Would it make sense to add a PeekNext() function to the iterator so that we
> don't have to queue the sample?

I tried to limit my changes to the MoofParser / Index code and do with what is currently available.
What I would need I think is the ability is to peek on the sample table and not the full demux (which is blocking).

Having said that, currently, our Seek is *always* followed by a call to demux.

> Shouldn't media::TimeUnit::emplace(TimeUnit) not need you to convert from a
> TimeUnit to a TimeUnit? i.e. I'm surprised you need to call FromMicroseconds
> on a TimeUnit when assigning a TimeUnit to a TimeUnit.

Well, that's where I love names. across namespaces !
mp4_demuxer::Microseconds is a typedef on int64_t :) 

A TimeUnit can be constructed from a media::Microseconds , but not an int64_t (or mp4_demuxer::Microseconds)...

Makes for confusing code all those Microseconds everywhere (and there's one defined differently in webrtc which is a double)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a525aad9de
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c9b5468e31
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d58f20f7252
https://hg.mozilla.org/integration/mozilla-inbound/rev/456b71b199de
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff00018ff1b

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6a525aad9de
https://hg.mozilla.org/mozilla-central/rev/66c9b5468e31
https://hg.mozilla.org/mozilla-central/rev/4d58f20f7252
https://hg.mozilla.org/mozilla-central/rev/456b71b199de
https://hg.mozilla.org/mozilla-central/rev/9ff00018ff1b