Closed Bug 1118123 Opened 6 years ago Closed 6 years ago

MSE: Appending data to source buffer do not update MediaSource duration

Categories

(Core :: Audio/Video, defect, P2)

defect

Tracking

()

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

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 2 obsolete files)

When appending data to a buffer, the mediasource duration attribute should be updated with the new duration.

The attached test case shows that after appending 10s of video, mediaSource.duration attribute is still 0.

From the spec:
"appendBuffer(), appendStream() and endOfStream() can update the duration under certain circumstances."

This will call the Coded Frame Processing Algorithm
http://w3c.github.io/media-source/#sourcebuffer-coded-frame-processing

"If the media segment contains data beyond the current duration, then run the duration change algorithm with new duration set to the maximum of the current duration and the group end timestamp."
Assignee: nobody → jyavenard
Priority: -- → P2
Update mediasource duration as per spec following call to appendBuffer
Attachment #8545461 - Flags: review?(cajbir.bugzilla)
mochitest to verify behaviour. Some hoops to do so as loading the init segment would set an original duration of 4s. So we have to wait for that duration to change, then change it back to 0. And only then call appendBuffer
Attachment #8545474 - Flags: review?(cajbir.bugzilla)
Update web reftest to reflect changes. Sounds like more could be re-enabled
Attachment #8545475 - Flags: review?(karlt)
Attachment #8545461 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8545474 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8545475 - Flags: review?(karlt) → review?(cajbir.bugzilla)
Attachment #8545475 - Flags: review?(cajbir.bugzilla) → review+
Carrying r+
Attachment #8545475 - Attachment is obsolete: true
my hg queue got messed up for some reasons
Attachment #8546292 - Attachment is obsolete: true
Comment on attachment 8545461 [details] [diff] [review]
Update mediasource duration following sourcebuffer::appendBuffer

Landed the first two patches on Aurora with a=mse pre-approval.

https://hg.mozilla.org/releases/mozilla-aurora/rev/dda3f8613738
https://hg.mozilla.org/releases/mozilla-aurora/rev/cfe3df775521

Skipped the third patch, as it adjusts web platform test changes which we haven't been uplifting.

Main risk is that this will change test results and we haven't uplifted the new expectations. Code changes are MSE-specific.
Attachment #8545461 - Flags: approval-mozilla-aurora?
Ralph, I would actually back them out..

There is a possibility (albeit rare) that CheckEndTime() is called from RangeRemoval: in which case we would either assert as mUpdating is true; or would end up issuing two update/updateend in a row (with no matching updatestart)...

Currently the issue is mostly hidden because it's hidden by another bug (we always treat the initial duration as 0, and it only ever grows under most circumstances).
But bug 1119757 and bug 1118126 expose them more.

I have a fix. Ran out of time to commit them
Flags: needinfo?(giles)
Ok, backed out. We'll reland with the fixes from bug 1118126 when they're ready.

https://hg.mozilla.org/releases/mozilla-aurora/rev/893ed355ee54
Depends on: 1118126
Flags: needinfo?(giles)
These also caused mediasource-config-change-webm-v-bitrate.html test failures on the Aurora push. Possibly missing dependent patch.
ah yes, the need 1118123_web_platform_fix.patch applied.
Comment on attachment 8545461 [details] [diff] [review]
Update mediasource duration following sourcebuffer::appendBuffer

reset for now
Attachment #8545461 - Flags: approval-mozilla-aurora?
Jean-Yves, are we still waiting on bug 1118126 before uplifting this? It was on your 'nice to have' list for beta 2, but in comment #9 you said to wait.
Flags: needinfo?(jyavenard)
Comment on attachment 8545461 [details] [diff] [review]
Update mediasource duration following sourcebuffer::appendBuffer

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, subsequent fixes don't backport cleanly.
[Describe test coverage new/current, TBPL]: Landed on m-c, m-a.
[Risks and why]: Risk is low; we've had no problem with these patches on 37.
[String/UUID change made/needed]: None.

This request is for all patches on this bug. Jean-Yves doesn't seem to be around, but re-nominating for beta anyway. I think it's better to be consistent between the three branches at this point, so this should go in.
Attachment #8545461 - Flags: approval-mozilla-beta?
(In reply to Ralph Giles (:rillian) from comment #14)
> Jean-Yves, are we still waiting on bug 1118126 before uplifting this? It was
> on your 'nice to have' list for beta 2, but in comment #9 you said to wait.

bug 1118126 is the range removal implementation we won't be ready for another day or say from my side.

bug 1120079 is what prevents the problem from occurring. Problem with that one is that it relies on a whole heaps.

My list of bug to uplift were in regards to Beta 2. Now that we've passed that deadline, and we're for Beta 3 I think all of them should be uplifted.
Flags: needinfo?(jyavenard)
Comment on attachment 8545461 [details] [diff] [review]
Update mediasource duration following sourcebuffer::appendBuffer

It is not too late for beta 2. A bunch of patches didn't land.
Attachment #8545461 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Appears to have test automation coverage.
Flags: qe-verify-
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.