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

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1301307: [MSE] Throw error when sourcebuffer is full and no data could be evicted.

Review commit: https://reviewboard.mozilla.org/r/77590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77590/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 3

[Jean-Yves Avenard [:jya]]

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.

Comment 4

[Jean-Yves Avenard [:jya]]

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

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b535fb46742b