Closed Bug 1159027 Opened 5 years ago Closed 5 years ago

Create new MP4Demuxer, a MediaDataDemuxer object

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(5 files, 2 obsolete files)

This object would follow the new MediaDataDemuxer API as described in bug 1154164
Blocks: 1154164
Add method to retrieve position of the MP4 moov in the stream
Attachment #8602446 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Make BufferStream able to work with a MediaLargeByteBuffer fallible array
Attachment #8602448 - Flags: review?(ajones)
Attached patch Part3. Add MP4Demuxer object (obsolete) — Splinter Review
Add MP4Demuxer object
Attachment #8602451 - Flags: review?(cpearce)
Attachment #8602448 - Flags: review?(ajones) → review+
Implement SkipToNextRandomAccessPoint for MP4Demuxer
Attachment #8602716 - Flags: review?(cpearce)
Comment on attachment 8602451 [details] [diff] [review]
Part3. Add MP4Demuxer object

Will merge p3 and p4 together
Attachment #8602451 - Attachment is obsolete: true
Attachment #8602451 - Flags: review?(cpearce)
Attachment #8602716 - Attachment is obsolete: true
Attachment #8602716 - Flags: review?(cpearce)
MP4Demuxer object.
Attachment #8603120 - Flags: review?(cpearce)
An init segment is a ftyp *and* a moov as per spec. Otherwise it may cause stagefright to fail parsing the moov
Attachment #8603170 - Flags: review?(ajones)
Attachment #8603170 - Flags: review?(ajones) → review+
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);
Implement MP4Demuxer::NotifyDataArrived. We'll need it afterall
Attachment #8603893 - Flags: review?(cpearce)
Attachment #8602446 - Flags: review?(cpearce) → review+
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.
Attachment #8603120 - Flags: review?(cpearce) → review+
Attachment #8603893 - Flags: review?(cpearce) → review+
(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)
No longer blocks: 1154164
Depends on: 1154164
Depends on: 1200326
You need to log in before you can comment on or make changes to this bug.