Closed
Bug 1159027
Opened 7 years ago
Closed 7 years ago
Create new MP4Demuxer, a MediaDataDemuxer object
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: jya, Assigned: jya)
References
Details
Attachments
(5 files, 2 obsolete files)
3.37 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
4.51 KB,
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
14.11 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
4.75 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
This object would follow the new MediaDataDemuxer API as described in bug 1154164
Assignee | ||
Comment 1•7 years ago
|
||
Add method to retrieve position of the MP4 moov in the stream
Attachment #8602446 -
Flags: review?(cpearce)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Make BufferStream able to work with a MediaLargeByteBuffer fallible array
Attachment #8602448 -
Flags: review?(ajones)
Assignee | ||
Comment 3•7 years ago
|
||
Add MP4Demuxer object
Attachment #8602451 -
Flags: review?(cpearce)
Updated•7 years ago
|
Attachment #8602448 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Implement SkipToNextRandomAccessPoint for MP4Demuxer
Attachment #8602716 -
Flags: review?(cpearce)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8602716 -
Attachment is obsolete: true
Attachment #8602716 -
Flags: review?(cpearce)
Assignee | ||
Comment 7•7 years ago
|
||
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)
Updated•7 years ago
|
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);
Assignee | ||
Comment 9•7 years ago
|
||
Implement MP4Demuxer::NotifyDataArrived. We'll need it afterall
Attachment #8603893 -
Flags: review?(cpearce)
Updated•7 years ago
|
Attachment #8602446 -
Flags: review?(cpearce) → review+
Comment 10•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8603893 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 11•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•