Closed
Bug 1098126
Opened 10 years ago
Closed 9 years ago
[MSE] Make MPEG4Extractor able to cache moof locations
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: ajones, Assigned: ajones)
References
(Blocks 1 open bug)
Details
Attachments
(7 files, 3 obsolete files)
8.07 KB,
patch
|
mattwoodrow
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
10.85 KB,
patch
|
mattwoodrow
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
5.90 KB,
patch
|
mattwoodrow
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
20.23 KB,
patch
|
eflores
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.37 KB,
patch
|
mattwoodrow
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.88 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
4.43 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
MPEG4Extractor doesn't keep a list of moof locations or parse new data when it arrives. This could cause us to stall because we have a buffered range but aren't able to seek into it because we can't find the start of the fragment.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8525005 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ajones
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8525026 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8525026 [details] [diff] [review] Seek in fMP4 buffered ranges Hmm.. seems to be crashing...
Attachment #8525026 -
Flags: review?(matt.woodrow)
Comment 4•10 years ago
|
||
Comment on attachment 8525005 [details] [diff] [review] Remove duplication in MP4 demuxer seek Review of attachment 8525005 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp @@ -3636,5 @@ > - > - mCurrentSamples.clear(); > - mDeferredSaio.clear(); > - mDeferredSaiz.clear(); > - mCurrentSampleIndex = 0; Why don't we need to clear these any more?
Attachment #8525005 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > Comment on attachment 8525005 [details] [diff] [review] > Remove duplication in MP4 demuxer seek > > Review of attachment 8525005 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp > @@ -3636,5 @@ > > - > > - mCurrentSamples.clear(); > > - mDeferredSaio.clear(); > > - mDeferredSaiz.clear(); > > - mCurrentSampleIndex = 0; > > Why don't we need to clear these any more? I moved them back to the place they used to be for the top half of the 'if' statement and moved them into the function for the bottom half.
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8529401 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8529404 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Attachment #8525026 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8529509 -
Flags: review?(matt.woodrow)
Comment 9•9 years ago
|
||
Comment on attachment 8529404 [details] [diff] [review] Use MoofParser to read fragmented MP4 data Review of attachment 8529404 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/Index.cpp @@ +81,5 @@ > + , mCurrentSample(0) > +{ > +} > + > +MP4Sample* SampleIterator::GetNext() Can we use nsAutoPtr<MP4Sample> as the return value to avoid leaving a bare pointer untracked? @@ +163,2 @@ > Index::Index(const stagefright::Vector<MediaSource::Indice>& aIndex, > + Stream* aSource, uint32_t aTrackId) : mSource(aSource) Put the initializer on a new line.
Attachment #8529404 -
Flags: review?(matt.woodrow) → review+
Updated•9 years ago
|
Attachment #8529509 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8530057 -
Flags: review?(matt.woodrow)
Updated•9 years ago
|
Attachment #8530057 -
Flags: review?(matt.woodrow) → review?(edwin)
Comment on attachment 8530057 [details] [diff] [review] Add CENC support to MoofParser Review of attachment 8530057 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/libstagefright/binding/MoofParser.cpp @@ +495,5 @@ > + > + if (flags & 1) { > + mAuxInfoType = reader->ReadU32(); > + mAuxInfoTypeParameter = reader->ReadU32(); > + } Per spec, mAuxInfoType should default to the sinf scheme type or sample entry type. @@ +516,5 @@ > + > + if (flags & 1) { > + mAuxInfoType = reader->ReadU32(); > + mAuxInfoTypeParameter = reader->ReadU32(); > + } Same as above.
Attachment #8530057 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8532663 -
Flags: review?(giles)
Comment 13•9 years ago
|
||
Comment on attachment 8532663 [details] [diff] [review] Disable MoofParser for EME Review of attachment 8532663 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8532663 -
Flags: review?(giles) → review+
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84423f167adf
Comment 15•9 years ago
|
||
This is hitting Windows webplatform test failures: https://treeherder.mozilla.org/logviewer.html#?job_id=4406640&repo=mozilla-inbound I'm heading back to my hotel room now. I'll back this out if it hasn't been dealt with before then.
Flags: needinfo?(ajones)
Comment 16•9 years ago
|
||
Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/ea02960b7856
Flags: needinfo?(ajones)
Comment 17•9 years ago
|
||
Also non-unified bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=4407787&repo=mozilla-inbound
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8b11d1e93b https://hg.mozilla.org/integration/mozilla-inbound/rev/36f3e9c34b70 https://hg.mozilla.org/integration/mozilla-inbound/rev/4e7a358bdd20 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8bb6132ae1 https://hg.mozilla.org/integration/mozilla-inbound/rev/81bdd9d3aa25
Comment 19•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/225f36741b2f - those Windows opt w-p-t-4 failures on try? They were real.
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8535380 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Attachment #8532663 -
Attachment is obsolete: true
Comment 21•9 years ago
|
||
Comment on attachment 8535380 [details] [diff] [review] MoofParser fixes and disable for EME Review of attachment 8535380 [details] [diff] [review]: ----------------------------------------------------------------- I think we want to reset the demuxer position for WebMReader too at least. A followup is fine.
Attachment #8535380 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 22•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a1b789efb21
Comment 23•9 years ago
|
||
Comment on attachment 8525005 [details] [diff] [review] Remove duplication in MP4 demuxer seek Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent playback support, video sites more likely to use flash. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: This change is limited to fragmented mp4 handling which is specific to mse, so regression risk for normal <video> playback is low. [String/UUID change made/needed]: None
Attachment #8525005 -
Flags: approval-mozilla-aurora?
Comment 24•9 years ago
|
||
Comment on attachment 8529404 [details] [diff] [review] Use MoofParser to read fragmented MP4 data Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent playback support, video sites more likely to use flash. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: This change is limited to fragmented mp4 handling which is specific to mse, so regression risk for normal <video> playback is low. [String/UUID change made/needed]: None
Attachment #8529404 -
Flags: approval-mozilla-aurora?
Comment 25•9 years ago
|
||
Comment on attachment 8529509 [details] [diff] [review] MoofParser forced moof read Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent playback support, video sites more likely to use flash. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: This change is limited to fragmented mp4 handling which is specific to mse, so regression risk for normal <video> playback is low. [String/UUID change made/needed]: None
Attachment #8529509 -
Flags: approval-mozilla-aurora?
Comment 26•9 years ago
|
||
Comment on attachment 8530057 [details] [diff] [review] Add CENC support to MoofParser Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent playback support, video sites more likely to use flash. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: This change is limited to fragmented mp4 handling which is specific to mse, so regression risk for normal <video> playback is low. [String/UUID change made/needed]: None
Attachment #8530057 -
Flags: approval-mozilla-aurora?
Comment 27•9 years ago
|
||
Comment on attachment 8535380 [details] [diff] [review] MoofParser fixes and disable for EME Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent playback support, video sites more likely to use flash. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: This change is limited to fragmented mp4 handling which is specific to mse, so regression risk for normal <video> playback is low. [String/UUID change made/needed]: None
Attachment #8535380 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 28•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=532e8cfcc868
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8536315 -
Flags: review?(cajbir.bugzilla)
Comment 30•9 years ago
|
||
Comment on attachment 8536315 [details] [diff] [review] Disable intermittent web platform tests for MSE Review of attachment 8536315 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/meta/media-source/mediasource-duration.html.ini @@ +1,3 @@ > [mediasource-duration.html] > type: testharness > + disabled: TIMEOUT or FAIL https://bugzilla.mozilla.org/show_bug.cgi?id=1085247 Remove trailing whitespace.
Attachment #8536315 -
Flags: review?(cajbir.bugzilla) → review+
Assignee | ||
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3830d09ce0e https://hg.mozilla.org/integration/mozilla-inbound/rev/3af0cab9d0de https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecd5b8c293a https://hg.mozilla.org/integration/mozilla-inbound/rev/732d6e4e6bed https://hg.mozilla.org/integration/mozilla-inbound/rev/7da16258c33b https://hg.mozilla.org/integration/mozilla-inbound/rev/05ca5d4570e8
Comment 32•9 years ago
|
||
sorry had to back this out for possibly causing bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4642794&repo=mozilla-inbound
Comment 33•9 years ago
|
||
this failure might be related too: https://treeherder.mozilla.org/logviewer.html#?job_id=4642276&repo=mozilla-inbound
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → affected
Assignee | ||
Comment 34•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8498a24cd12f
Assignee | ||
Comment 35•9 years ago
|
||
Attachment #8536928 -
Flags: review?(matt.woodrow)
Updated•9 years ago
|
Attachment #8536928 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 36•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/117b38a298ab https://hg.mozilla.org/integration/mozilla-inbound/rev/9d83f03575b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d13ae622080 https://hg.mozilla.org/integration/mozilla-inbound/rev/73275f4fe252 https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb3299f2a30 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f64e4663be https://hg.mozilla.org/integration/mozilla-inbound/rev/20f481cfd2d9
Comment 37•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/117b38a298ab https://hg.mozilla.org/mozilla-central/rev/9d83f03575b9 https://hg.mozilla.org/mozilla-central/rev/4d13ae622080 https://hg.mozilla.org/mozilla-central/rev/73275f4fe252 https://hg.mozilla.org/mozilla-central/rev/0cb3299f2a30 https://hg.mozilla.org/mozilla-central/rev/a2f64e4663be https://hg.mozilla.org/mozilla-central/rev/20f481cfd2d9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8525005 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8529404 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8529509 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8530057 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8535380 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 38•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/605b0bf08088 https://hg.mozilla.org/releases/mozilla-aurora/rev/32cfcdbc5dd7 https://hg.mozilla.org/releases/mozilla-aurora/rev/0b89600c76db https://hg.mozilla.org/releases/mozilla-aurora/rev/a409b87264ea https://hg.mozilla.org/releases/mozilla-aurora/rev/fae25678594d https://hg.mozilla.org/releases/mozilla-aurora/rev/582bc919c315 https://hg.mozilla.org/releases/mozilla-aurora/rev/d5a209f1d1ce In the future, please try to roll bustage fixes into the original patches rather than stacking a "fix bustage" patch on top. It breaks bisection.
Assignee | ||
Comment 39•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #38) > In the future, please try to roll bustage fixes into the original patches > rather than stacking a "fix bustage" patch on top. It breaks bisection. Will do. In this case it is the non-default build flag introduced in 1009631.
Comment 40•9 years ago
|
||
Re-enabled web platform tests that were disabled in a2f64e4663be with new results, which seem to be stable. https://tbpl.mozilla.org/?tree=Try&rev=30323dc5e527 https://hg.mozilla.org/integration/mozilla-inbound/rev/745e52e4f7ba
Updated•9 years ago
|
Keywords: checkin-needed
Whiteboard: [please uplift 745e52e4f7ba to 36/beta]
Comment 42•9 years ago
|
||
Ralph took care of it.
Keywords: checkin-needed
Whiteboard: [please uplift 745e52e4f7ba to 36/beta]
You need to log in
before you can comment on or make changes to this bug.
Description
•