1302465 - [MSE] QuotaExceededError is still being thrown if data at the front can be evicted

Status: RESOLVED FIXED

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

Description

[xqq]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2853.0 Safari/537.36

Steps to reproduce:

In bug 1280023, new eviction policy has been implemented.
Now QuotaExceededError will be thrown if buffer full and no data can be evicted.

Assume an scene that buffer has been full and data at the front cannot be evicted. At this time, appendBuffer() call can cause QuotaExceededError;

But when currentTime close to the end, appendBuffer() call can still cause QuotaExceededError exception.


Actual results:

When currentTime close to the end, appendBuffer() call can still cause QuotaExceededError exception.


Expected results:

When currentTime close to the end, data at the front can be evicted safely. So appendBuffer() call should automatically evict data at the front to make space for new buffer, rather than throw QuotaExceededError all the time.

When appendBuffer() called, Coded Frame Eviction Algorithm should be run during the Prepare Append Algorithm first, then check buffer full flag to determine whether to throw a QuotaExceededError. 

See https://w3c.github.io/media-source/#sourcebuffer-prepare-append

Comment 1

[Jean-Yves Avenard [:jya]]

As I mentioned in the other bug. The situation you describe can't happen.
QuotaExceededErrror will only be fired if the source buffer size is over its threshold.

If currentTime is close to the end and data can be evicted at the front, then it will be evicted before anything else.

Please provide a example showing the situation you describe, as I don't see how it could happen.

Otherwise I believe this bug should be closed as invalid.

Comment 2

[Jean-Yves Avenard [:jya]]

Hmmm.. Thinking further about this, there may be a side case where we would throw the error but don't attempt to evict... We should try again...

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1302465: [MSE] Schedule eviction, even if previously the source buffer was full.

Conditions may have changed (such as currentTime moving since the last attempt). So we try again.

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

Comment 4

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

Comment on attachment 8791002 [details]
Bug 1302465: [MSE] Schedule eviction, even if previously the source buffer was full.

https://reviewboard.mozilla.org/r/78566/#review77156

r+ with comments fixed:

::: dom/media/mediasource/TrackBuffersManager.cpp:293
(Diff revision 1)
> -  }
> -
> -  MSE_DEBUG("Reaching our size limit, schedule eviction of %lld bytes", toEvict);
>  
> +  if (mBufferFull && mEvictionState == EvictionState::EVICTION_COMPLETED) {
> +    // Our buffer is currently full. We will attempt another eviction attempt.

'attempt another ... attempt' -> 'make another ... attempt'

::: dom/media/mediasource/TrackBuffersManager.cpp:295
(Diff revision 1)
> -  MSE_DEBUG("Reaching our size limit, schedule eviction of %lld bytes", toEvict);
>  
> +  if (mBufferFull && mEvictionState == EvictionState::EVICTION_COMPLETED) {
> +    // Our buffer is currently full. We will attempt another eviction attempt.
> +    // However, the current appendBuffer will fail as we can't know ahead of
> +    // time if eviction will later succeed.

'eviction' -> 'the eviction'

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8791002 [details]
Bug 1302465: [MSE] Schedule eviction, even if previously the source buffer was full.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78566/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1c46f438bbf
[MSE] Schedule eviction, even if previously the source buffer was full. r=gerald

Comment 7

[xqq]

Since our video player has not been opened source, it's hard to provide a reproduceable example for now.
Maybe open source in the near future.

Comment 8

[Jean-Yves Avenard [:jya]]

with the current change, it is possible the an appendBuffer will be rejected even though eviction could occur.

The reason for this, is as per spec. Data is never evicted until the prepare append algorithm is called; which in the spec is required to be synchronous.

However, everything else in the spec is asynchronous, removal of data synchronously is incompatible with our architecture.

So if you get a QuotaExceededError, then move currentTime (either by letting the file play or seeking). The next appendBuffer will once again fail. However the one after that will succeed.

Bug 1302573 aims to improve that situation.

The most effective way to remove data is to manually call remove() (which is what youtube is doing prior seeking, it clear the sourcebuffer prior the seeking target). It's the most relivable way to get things done the way you want to.

Comment 9

[xqq]

I don't think synchronized Coded Frame Eviction could cause a performance bottleneck...

Chrome/IE/Edge is always working as expect in the above topics.

Implementation should conform to spec strictly which is important for the HTML5 MSE developer. Apply various workarounds for different browser brings a lot of trouble and dirty code, which I have been tired with that...


At last, the use of MSE is not restricted to adaptive streaming (like DASH, or HLS) only. Developers may use MSE for custom format playback, such as traditional video file playback which browser doesn't support natively.

Developers may do buffering as much as possible for reasonable purpose, which is not lazy load like DASH. In this case, monitoring QuotaExceededError is the only way to detect buffer full state. When playback progress near the end of buffered-range, download task will be resume and new segment will be append, data at the front can be evicted safely so new segment should be appended successfully.

So in my opinion QuotaExceededError should be thrown conformed to spec :)

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to xqq from comment #9)
> I don't think synchronized Coded Frame Eviction could cause a performance
> bottleneck...

in a multi-threaded work, you certainly don't want your main thread to be blocking.
In fact there are plenty of things incorrectly defined in the spec when it comes to the handling of asynchronous operations and synchronous ones (like the call to abort() supposed to abort something we have no way of telling how much has actually already occurred)

Your comment implies that you have no knowledge on thread-safety nor how our implementation work.

> 
> Chrome/IE/Edge is always working as expect in the above topics.


> 
> Implementation should conform to spec strictly which is important for the
> HTML5 MSE developer. Apply various workarounds for different browser brings
> a lot of trouble and dirty code, which I have been tired with that...

Firefox comply to the spec more strictly than any browsers out there...

There's also nothing in the spec that states what is to be evicted nor how it should be evicted.

So we are still 100% spec compliant on the matter.

You are expecting eviction to work in a certain way because that's how chrome chose to implement it. Not that their implementations is the best there is or the only way it can be done.

> Developers may do buffering as much as possible for reasonable purpose,
> which is not lazy load like DASH. In this case, monitoring
> QuotaExceededError is the only way to detect buffer full state. When
> playback progress near the end of buffered-range, download task will be
> resume and new segment will be append, data at the front can be evicted
> safely so new segment should be appended successfully.

you're making invalid assumptions on what can be evicted and at what time. None of those assumptions are defined in the MSE specs.

Your application assume on how the user agent is working internally, that is wrong. If you do so, your code will never work across all platforms.

Buffering all the data because you can is also an invalid approach; what if the platforms you're using has a much lower memory threshold than others? like on a mobile.

What if eviction isn't done based on the number of bytes available, but on the maximum TIME the source buffer allow. That too would be spec compliant (and it's actually one of the approach we are considering: forget about a 10MB or 100MB sourcebuffer size, instead allow a maximum of 30s of buffered video)

> 
> So in my opinion QuotaExceededError should be thrown conformed to spec :)

and it is...
it is fired when the buffer is full, exactly as per spec.

Again, you are assuming on what can be evicted and when eviction should occur; when there's no specifications for those anywhere.

The only the spec assumes (and which I believe is wrong btw), is that eviction is only to occur when you're attempting to add new data.
We intend to lodge a spec bug that would allow eviction to occur at any time.

If you want to control how the data is buffered, and what can be removed. MSE gives you two tools for that:
1- reading the buffered attribute
2- use remove()

Those are the only two methods that are GUARANTEED to work and are fully specified.

You know when your application will seek, and you know when currentTime progress (through the timeupdate event)....
If you want eviction to occur like you think it should -> use those.

Comment 11

[xqq]

I agree that many details are not included in MSE spec, and some may be unfriendly for implementation side.

For the previous bug 1279885, I agree that this is compliant with spec, because eviction policy is not included in spec. But it doesn't means firefox's policy is an **reasonable** policy. It is a strange and incorrect policy.

I understood that these are not spec-inconformity problem, sorry for my expression... But even though you emphasized it is 100% compliant with spec, it is still an unreasonable policy which should be fixed or improved.


At last, QuotaExceededError repetition which descripted in this issue is a bug which is not spec compliant.
Hopes to be fixed in the future, or a better solution to be introduced.

Comment 12

[Jean-Yves Avenard [:jya]]

I'm not sure what you are arguing about.. this issue has been fixed and is pending...

Comment 13

[xqq]

You said that under the current change, It is still possible the an appendBuffer will be rejected even though eviction could occur

Comment 14

[Carsten Book [:Tomcat]]

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