Add Test for multiple init segments in MSE appendBuffer

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: j, Assigned: j)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Move test from bug 1184867 and add same test for mp4.
Currently loadedmetadata is not triggered in both tests.
Attachment #8639831 - Flags: review?(jyavenard)
(Assignee)

Updated

4 years ago
Assignee: nobody → j
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.
I'm going to lodge a bug for the MSE spec ; they don't properly handle that case
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.
Attachment #8639831 - Flags: review?(jyavenard) → review-
The name IsInitSegmentPresent and IsMediaSegmentPresent was misleading. As they are to return true only if data *starts* with such segment.
Attachment #8640305 - Flags: review?(ajones)
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.
Attachment #8640306 - Flags: review?(gsquelart)
Depends on: 1174577
Attachment #8640306 - Flags: review?(gsquelart) → review+
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.
Attachment #8640305 - Flags: review?(ajones) → review-
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 :(
Flags: needinfo?(ajones)
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.
Flags: needinfo?(ajones)
Perhaps you could just return a struct instead of a bool and an out-param?
The name IsInitSegmentPresent and IsMediaSegmentPresent was misleading. As they are to return true only if data *starts* with such segment and not just contain.
Attachment #8640394 - Flags: review?(ajones)
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.
Attachment #8640394 - Flags: review?(ajones) → review+
(Assignee)

Comment 14

4 years ago
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’
Depends on: 1188804
Comment on attachment 8640305 [details] [diff] [review]
P2. Ensure data starts with and not just contains.

moving to bug 1188804
Attachment #8640305 - Attachment is obsolete: true
Comment on attachment 8640306 [details] [diff] [review]
P3. Disambiguate naming of mActiveTrack boolean.

moving to bug 1188804
Attachment #8640306 - Attachment is obsolete: true
Comment on attachment 8640394 [details] [diff] [review]
[MSE] P2. Fix Is*SegmentPresent.

moving to bug 1188804
Attachment #8640394 - Attachment is obsolete: true
(Assignee)

Comment 18

4 years ago
using load loadSegment helper and durationchange
Attachment #8639831 - Attachment is obsolete: true
Attachment #8640495 - Flags: review?(jyavenard)
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
Attachment #8640495 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 20

4 years ago
merging patches and adding a comment. carrying r+
Attachment #8640307 - Attachment is obsolete: true
Attachment #8640495 - Attachment is obsolete: true
Attachment #8640515 - Flags: review+
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
});
(Assignee)

Comment 23

4 years ago
(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.
it is !

jumped the gun too quickly.

And it's one of my patch that put the regression. not a good day for me
https://hg.mozilla.org/mozilla-central/rev/734cc0057221
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.