Closed Bug 1098126 Opened 10 years ago Closed 9 years ago

[MSE] Make MPEG4Extractor able to cache moof locations

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: ajones, Assigned: ajones)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

8.07 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
10.85 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.90 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
20.23 KB, patch
eflores
: review+
Details | Diff | Splinter Review
4.37 KB, patch
mattwoodrow
: review+
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: nobody → ajones
Status: NEW → ASSIGNED
Attached patch Seek in fMP4 buffered ranges (obsolete) — Splinter Review
Attachment #8525026 - Flags: review?(matt.woodrow)
Comment on attachment 8525026 [details] [diff] [review]
Seek in fMP4 buffered ranges

Hmm.. seems to be crashing...
Attachment #8525026 - Flags: review?(matt.woodrow)
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+
(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.
Attachment #8529401 - Attachment is obsolete: true
Attachment #8529404 - Flags: review?(matt.woodrow)
Attachment #8525026 - Attachment is obsolete: true
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+
Attachment #8529509 - Flags: review?(matt.woodrow) → review+
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+
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+
Blocks: 1108059
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)
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.
Attachment #8532663 - Attachment is obsolete: true
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+
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 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 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 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 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?
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+
sorry had to back this out for possibly causing bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4642794&repo=mozilla-inbound
Attachment #8536928 - Flags: review?(matt.woodrow)
Attachment #8536928 - Flags: review?(matt.woodrow) → review+
Attachment #8525005 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8529404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8529509 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8530057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8535380 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
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
Keywords: checkin-needed
Whiteboard: [please uplift 745e52e4f7ba to 36/beta]
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.