1098126 - [MSE] Make MPEG4Extractor able to cache moof locations

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

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.

Comment 1

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

Attachment: Remove duplication in MP4 demuxer seek

Comment 2

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

Attachment: Seek in fMP4 buffered ranges

Comment 3

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

Comment on attachment 8525026 [details] [diff] [review]
Seek in fMP4 buffered ranges

Hmm.. seems to be crashing...

Comment 4

[Matt Woodrow (:mattwoodrow)]

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?

Comment 5

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

(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.

Comment 6

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

Attachment: Use MoofParser to read fragmented MP4 data

Comment 7

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

Attachment: Use MoofParser to read fragmented MP4 data

Comment 8

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

Attachment: MoofParser forced moof read

Comment 9

[Matt Woodrow (:mattwoodrow)]

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.

Comment 10

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

Attachment: Add CENC support to MoofParser

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [: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.

Comment 12

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

Attachment: Disable MoofParser for EME

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8532663 [details] [diff] [review]
Disable MoofParser for EME

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

lgtm

Comment 14

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84423f167adf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea02960b7856

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Also non-unified bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=4407787&repo=mozilla-inbound

Comment 18

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

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

[Phil Ringnalda (:philor)]

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.

Comment 20

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

Attachment: MoofParser fixes and disable for EME

Comment 21

[Matt Woodrow (:mattwoodrow)]

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.

Comment 22

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a1b789efb21

Comment 23

[Ralph Giles (:rillian)]

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

Comment 24

[Ralph Giles (:rillian)]

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

Comment 25

[Ralph Giles (:rillian)]

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

Comment 26

[Ralph Giles (:rillian)]

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

Comment 27

[Ralph Giles (:rillian)]

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

Comment 28

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=532e8cfcc868

Comment 29

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

Attachment: Disable intermittent web platform tests for MSE

Comment 30

[cajbir (:cajbir)]

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.

Comment 31

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

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

[Carsten Book [:Tomcat]]

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

[Carsten Book [:Tomcat]]

this failure might be related too:
https://treeherder.mozilla.org/logviewer.html#?job_id=4642276&repo=mozilla-inbound

Comment 34

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8498a24cd12f

Comment 35

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

Attachment: Fix build bustage

Comment 36

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

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

[Carsten Book [:Tomcat]]

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 39

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

(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

[Karl Tomlinson (:karlt)]

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

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/745e52e4f7ba

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Ralph took care of it.