[MSE] When sourceBuffer is full, QuotaExceeded exception will not be thrown

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
There's a logic error in the eviction code.

Before throwing QuotaExceeded we attempt to evict data. Some data may be evicted.
As soon as new data is added to the sourcebuffer successfully, the flag indicating that we need a run of eviction is cleared.

As the data is typically processed one MOOF/CLUSTER segment at a time (typically 4s worth of videos with youtube), while we can't add the full data, we can add it partially.

And so between the time we request eviction, and eviction gets to run, the flag has already been reset, and QuotaExceeded will never be fired as the TrackBufferManagers thinks it needs to try one more time
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8789316 [details]
Bug 1301307: [MSE] Throw error when sourcebuffer is full and no data could be evicted.

https://reviewboard.mozilla.org/r/77590/#review75866

r+ on the basis that it's a small change that seems to do the job, so it shouldn't be worse than before.

But these atomics used between two threads feel like a ticking timebomb -- even though I cannot see anything wrong (yet?)

I would suggest you open a bug to document how the whole machinery works, at best it could help uncover potential races, or at second best it would actually record this analysis somewhere so I/we don't need to cringe every time nearby code is being reviewed!
And maybe this exercise could give ideas on how to re-implement this in a less fragile-looking way.
Attachment #8789316 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 3

a year ago
mozreview-review-reply
Comment on attachment 8789316 [details]
Bug 1301307: [MSE] Throw error when sourcebuffer is full and no data could be evicted.

https://reviewboard.mozilla.org/r/77590/#review75866

How is that fragile looking?

Due to how MSE works, you can't have two appendBuffer going at once. A new one can only occur once a 2nd arrive.
So the boolean is checked to see if we've already had an eviction. Queue a task, that will change the state. TrackBuffersManager::EvictData can't be called until the previous task has completed.

As such, it doesn't even really need to be atomic. But because it's accessed on two threads, I made it atomic for the sake of it.
(Assignee)

Comment 4

a year ago
mozreview-review-reply
Comment on attachment 8789316 [details]
Bug 1301307: [MSE] Throw error when sourcebuffer is full and no data could be evicted.

https://reviewboard.mozilla.org/r/77590/#review75866

I meant "A new one can only occur once the previous completed" of course
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> How is that fragile looking?
[...]
> I made it atomic for the sake of it.
I rest my case.

But seriously, if you think the logic is such that both EvictData and the eviction task cannot possibly run at the same time, then you could try to prove it through with assertions.

Comment 6

a year ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b535fb46742b
[MSE] Throw error when sourcebuffer is full and no data could be evicted. r=gerald

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b535fb46742b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.