1188341 - Add Test for multiple init segments in MSE appendBuffer

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Jan Gerber]

Attachment: Bug-1188341-Add-Test-for-multiple-init-segments.patch

Move test from bug 1184867 and add same test for mp4.
Currently loadedmetadata is not triggered in both tests.

Comment 2

[Jean-Yves Avenard [:jya]]

I see why it fails.

The first init segment is parsed ; then removed from the stream. and SegmentParserLoop starts again.

But the next call to IsInitSegmentPresent() returns true even if there's a media segment before it.

So we skip the media segment and immediately parse the 2nd init segment.

And something else appears wrong too there, as the parser somehow can't parse the 2nd init segment.

thanks for that ! very useful.

Comment 3

[Jean-Yves Avenard [:jya]]

I'm going to lodge a bug for the MSE spec ; they don't properly handle that case

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639831 [details] [diff] [review]
Bug-1188341-Add-Test-for-multiple-init-segments.patch

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

I just got backed out on another similar test just two days ago due to intermittent failure.

explanation within...

::: dom/media/mediasource/test/test_MultipleInitSegments_mp4.html
@@ +41,5 @@
> +        });
> +      });
> +    });
> +    v.addEventListener("loadedmetadata", function () {
> +      ok(v.duration, 1.601666);

this is racy.

When "updateend" is fired, and when the metadata is processed ; the duration is the one as set by the init segment, and here it's ~10s.
There's no guarantee that v.duration will be 1.60s yet.

The call to ms.EndOfStream() will reduce the mediasource.duration to be of the highest time found in the sourcebuffer and another updatestart/update/updateend will be fired.
Only at that time is the duration guaranteed to be 1.60s.

You could use the promises helper found in mediasource.js to simplify the code, so you don't need updateCount and stuff like that.

Or change the test so that you wait for loadedmetadata to be fired, and inside that event handler you will call ms.endOfStream() ; in the sourcebuffer updateend event handler, when updateCount is > 1 can you test that duration is now 1.60s and complete the test (though it could be argued that really we should wait for the "durationchange" event.

Loading the init segment will cause one durationchange to be fired, and ms.endOfStream() because sb.buffered(0).end is < mediasource.duration , will also fire another durationchange.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P2. Ensure data starts with and not just contains.

The name IsInitSegmentPresent and IsMediaSegmentPresent was misleading. As they are to return true only if data *starts* with such segment.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: P3. Disambiguate naming of mActiveTrack boolean.

We considered that mActiveTrack was a global variable, however it is reset each time a new init segment is received.
Should two init segments be received in the same appendBuffer it would have been set to false, causing metadata to never be parsed.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Add missing mochitest info

Comment 8

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

Comment on attachment 8640305 [details] [diff] [review]
P2. Ensure data starts with and not just contains.

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

::: dom/media/mediasource/ContainerParser.cpp
@@ +261,5 @@
>      , mMonitor("MP4ContainerParser Index Monitor")
>    {}
>  
> +  bool HasAtom(const mp4_demuxer::AtomType& aAtom, const MediaByteBuffer* aData,
> +               uint32_t& aOffset) {

Why are we repeating logic here in HasAtom() instead of using Box? We should be using a class instead of an out value.

Comment 9

[Jean-Yves Avenard [:jya]]

Because using mp4_demuxer::Box would require significantly more code: setting up a stream to start with; that it reads the entire content which I don't care about (I only want to read the type).

To each job their tools, and mp4_demuxer::Box certainly isn't one appropriate for this specific usage.

We have already discussed this, the MoofParser and its components are unfortunately not the best for simply determining ranges ; and hence why ContainerParser has duplicated logic.

Plus now , you're pretty much asking me to rewrite everything you had r+ before :(

Comment 10

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

You've just changed how HasAtom() works so now it has an out parameter. Using a class is probably more appropriate now than it was before. It is starting to look messy so now is the time to clean it up. I'm not insisting that you use Box to do that.

Comment 11

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

Perhaps you could just return a struct instead of a bool and an out-param?

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P2. Fix Is*SegmentPresent.

The name IsInitSegmentPresent and IsMediaSegmentPresent was misleading. As they are to return true only if data *starts* with such segment and not just contain.

Comment 13

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

Comment on attachment 8640394 [details] [diff] [review]
[MSE] P2. Fix Is*SegmentPresent.

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

::: dom/media/mediasource/ContainerParser.cpp
@@ +285,5 @@
> +      const nsCString& mType(aType); // for logging macro.
> +      mp4_demuxer::ByteReader reader(aData);
> +      mp4_demuxer::AtomType initAtom("ftyp");
> +      mp4_demuxer::AtomType mediaAtom("moof");
> +      uint32_t offset = 0;

Use ByteReader::Offset() instead.

Comment 14

[Jan Gerber]

Comment on attachment 8640394 [details] [diff] [review]
[MSE] P2. Fix Is*SegmentPresent.

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

::: dom/media/mediasource/ContainerParser.cpp
@@ +281,5 @@
> +  class AtomParser {
> +  public:
> +    AtomParser(const nsACString& aType, const MediaByteBuffer* aData)
> +    {
> +      const nsCString& mType(aType); // for logging macro.

aType / nsAString vs mType / nsCString

error: invalid initialization of reference of type ‘const nsCString&’ from expression of type ‘const nsACString_internal’

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640305 [details] [diff] [review]
P2. Ensure data starts with and not just contains.

moving to bug 1188804

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640306 [details] [diff] [review]
P3. Disambiguate naming of mActiveTrack boolean.

moving to bug 1188804

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640394 [details] [diff] [review]
[MSE] P2. Fix Is*SegmentPresent.

moving to bug 1188804

Comment 18

[Jan Gerber]

Attachment: Bug-1188341-Add-Test-for-multiple-init-segments.patch

using load loadSegment helper and durationchange

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640495 [details] [diff] [review]
Bug-1188341-Add-Test-for-multiple-init-segments.patch

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

thanks for that.

Don't forget to include the content of the other patch I added ; otherwise those tests won't be used.

::: dom/media/mediasource/test/test_MultipleInitSegments_mp4.html
@@ +31,5 @@
> +            pos += buffer.byteLength;
> +          });
> +          loadSegment.bind(null, sb, arrayBuffer)().then(function() {
> +            v.addEventListener("durationchange", function () {
> +              ok(v.duration, 1.601666);

This is still potentially racy.
The first duration change may or may not be fired before the updateend (which loadSegment is based on). We do queue the first duration change before the updateend ; but this may be just our implementation. I would need to check.

you will get multiple duration changes over the course of this code: the first one from the initial parsing of the init segment (9.97s); then from the call to endOfStream() which reduces it to sb.buffered(0).end

Please add a comment that the first durationchange from parsing the init segment will be fired before updateend.

same for the webm test.

thanks

Comment 20

[Jan Gerber]

Attachment: Bug-1188341-Add-Test-for-multiple-init-segments.patch

merging patches and adding a comment. carrying r+

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/734cc0057221

Comment 22

[Jean-Yves Avenard [:jya]]

Oh well.. it was racy in the end.
https://treeherder.mozilla.org/logviewer.html#?job_id=12289202&repo=mozilla-inbound

so instead, let's do
loadSegment.bind(...).then(function() {
  var p = once(sb, 'updateend');
  ms.endOfStream();
  return p;
}).then(function() {
  ok(v.duration, 1.601666);
  SimpleTest.Finish
});

Comment 23

[Jan Gerber]

(In reply to Jean-Yves Avenard [:jya] from comment #22)
> Oh well.. it was racy in the end.
> https://treeherder.mozilla.org/logviewer.html#?job_id=12289202&repo=mozilla-
> inbound
> 

INFO TEST-UNEXPECTED-FAIL | dom/media/mediasource/test/test_BufferingWait_mp4.html | Test timed out.

that's another test that times out.

Comment 24

[Jean-Yves Avenard [:jya]]

it is !

jumped the gun too quickly.

And it's one of my patch that put the regression. not a good day for me

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/734cc0057221