Keep track on how much data can be evicted as currentTime progress

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51+ fixed, firefox52 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
One issue in our current eviction handling, is that we only know the result once it has occurred.

It means that we will always allow at least one extra appendBuffer that will cause our buffer to be full.
And we will know that data has been evicted only after it's been evicted.

By that time we would have thrown a QuotaExceeded error even though the data could have fitted in the buffer.

It would be ideal to keep track on which data can be evicted as we're playing it. That way we can estimate if a future appendBuffer will succeed before having to add it.

We simply need to track as GetSample and Seek are called. And assume that all samples prior the previous keyframe can be evicted at that point in time.
(Assignee)

Updated

3 years ago
See Also: → bug 1302465

Comment 1

3 years ago
It is too radical that assume all samples prior the previous keyframe can be evicted together at that point...

Video player may try to keep as much data as possible in buffered range for backward-seeking,
so lazy and conservative eviction may be better ?
(Assignee)

Comment 2

3 years ago
I didn't mean that all frames prior the key frame should be evicted... it would only be used to record how much data we can safely evict prior starting the eviction.

So that an appendBuffer can succeed as we know it will be able to fit later. 

Say for example that you've added 100MB of data, and now the buffer is full.
You have let currentTime progress to half the video duration. We can estimate that half the source buffer can be evicted without having to do the eviction.

So we allow the next appendBuffer to succeed (even though the sourcebuffer is still full) because we know the next eviction run will leave room.
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
The eviction change in bug 1302465 and bug 1280023; means that an appendBuffer could fail even though, per spec it should succeed.

As such, this change should be uplifted to 51 once ready.
status-firefox51: --- → affected
status-firefox52: --- → affected
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8793698 [details]
Bug 1302573: [MSE] P1. Add mochitest.

https://reviewboard.mozilla.org/r/80386/#review79050
Attachment #8793698 - Flags: review?(gsquelart) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8793699 [details]
Bug 1302573: [MSE] P2. Keep track of how much data can be evicted prior to current demuxing position.

https://reviewboard.mozilla.org/r/80388/#review79052

r+ with nits:

::: dom/media/mediasource/TrackBuffersManager.h:1
(Diff revision 2)
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In commit description:
"Keep track on" -> "Keep track of"
"prior" -> "prior to" (3 times)

::: dom/media/mediasource/TrackBuffersManager.h:350
(Diff revision 2)
> +      uint32_t mEvictable;
> +      uint32_t mLastIndex;
> +    };
> +    // Size of data that can be safely evicted during the next eviction
> +    // cycle.
> +    // We consider as evictable all frames up to the last keyframe prior

"prior" -> "prior to"

::: dom/media/mediasource/TrackBuffersManager.cpp:1983
(Diff revision 2)
> +  uint32_t i = aTrackData.mEvictionIndex.mLastIndex;
> +  TrackBuffer& data = aTrackData.mBuffers.LastElement();
> +
> +  for (; i < currentIndex; i++) {

Can't you do the def&init of 'i' inside the for init-statement?
Attachment #8793699 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8794478 [details]
Bug 1302573: [MSE] P3. Display evictable amount in about:media output.

https://reviewboard.mozilla.org/r/80928/#review79580
Attachment #8794478 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8794511 [details]
Bug 1302573: [MSE] P4. Be consistent in datatype being used.

https://reviewboard.mozilla.org/r/80934/#review79590

::: dom/media/mediasource/TrackBuffersManager.h:1
(Diff revision 1)
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In commit description:
"consistend" -> "consistent"
Attachment #8794511 - Flags: review?(gsquelart) → review+
(Assignee)

Updated

2 years ago
Blocks: 1305284
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8794596 [details]
Bug 1302573: [MSE] P5. Always evict data as soon as we can.

https://reviewboard.mozilla.org/r/80966/#review79596
Attachment #8794596 - Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

2 years ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7144f46515ed
[MSE] P1. Add mochitest. r=gerald
https://hg.mozilla.org/integration/autoland/rev/a41593ae3430
[MSE] P2. Keep track of how much data can be evicted prior to current demuxing position. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c0a30a0a2bcc
[MSE] P3. Display evictable amount in about:media output. r=gerald
https://hg.mozilla.org/integration/autoland/rev/426dfc90db8a
[MSE] P4. Be consistent in datatype being used. r=gerald
https://hg.mozilla.org/integration/autoland/rev/1dd2d2b5c5c8
[MSE] P5. Always evict data as soon as we can. r=gerald
(Assignee)

Updated

2 years ago
See Also: → bug 1305347
(Assignee)

Comment 35

2 years ago
ni? myself to remember to request uplifting at some stage
Flags: needinfo?(jyavenard)

Comment 36

2 years ago
Can it be tested under recent nightly build ?
(Assignee)

Comment 37

2 years ago
The next nightly should have it...

Otherwise you can use this:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd

Click on one of the green B next to the platform you want.
That will open a window on the bottom left, which links to the build.

For Windows 64 bits:
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win64-pgo/1474975804/firefox-52.0a1.en-US.win64.installer.exe
(Assignee)

Comment 38

2 years ago
[Tracking Requested - why for this release]: This causes bug 1305284 and is a YouTube playback regression
tracking-firefox51: --- → ?

Comment 39

2 years ago
It is now behaving as expect.

Thank you.
(Assignee)

Comment 40

2 years ago
\o/ thank you very much for reporting back.. Great to hear..

I can imagine corner cases where it could be assumed that data can be evicted but can't.
Such possible scenario would be this:
Append data until source buffer is almost full starting at t=10s (meaning the next appendBuffer would fill the source buffer)
Seek to t=10
Append data at t=0.
Append data at t=10 <--- buffer is full will be thrown
Append data again at t=10 <--- this one will work, we have evicted from o to 10.

The eviction prediction is only calculated after a seek or when video.playing = true; after a new appendData prior currentTime, the eviction prediction is reset and as such, if the buffer is full, we will always consider it full until the next run.
I don't think it's a major issue... The only way to be 100% per spec here would be to introduce locking between threads and we perform the eviction synchronously.

There's also the case where we could refuse an append buffer because we consider the source buffer full, even though the new frames would replace existing ones. As such, not adding space.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 41

2 years ago
Comment on attachment 8793698 [details]
Bug 1302573: [MSE] P1. Add mochitest.

Request is for all patches.

Approval Request Comment
[Feature/regressing bug #]: 1302465
[User impact if declined]: Sites using MSE can yield decode error, or cause audio distortion (as seen in bug 1305284
[Describe test coverage new/current, TreeHerder]: Manual test, reported as fixed. In central for a couple of weeks.
[Risks and why]: Low, it fixes an important regression affecting YouTube. Any further regressions will be quickly reported as it affects extremely popular sites.
[String/UUID change made/needed]: None
Attachment #8793698 - Flags: approval-mozilla-aurora?
Comment on attachment 8793698 [details]
Bug 1302573: [MSE] P1. Add mochitest.

Fix issues related to MSE. Take all patches in 51 aurora.
Attachment #8793698 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Track 51+ as regression related to youtube.
tracking-firefox51: ? → +
has problems uplifting to aurora like 

grafting 366381:a41593ae3430 "Bug 1302573: [MSE] P2. Keep track of how much data can be evicted prior to current demuxing position. r=gerald"
merging dom/media/mediasource/TrackBuffersManager.cpp
merging dom/media/mediasource/TrackBuffersManager.h
warning: conflicts while merging dom/media/mediasource/TrackBuffersManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jyavenard)
(In reply to Carsten Book [:Tomcat] from comment #44)
> has problems uplifting to aurora like 
> 
> grafting 366381:a41593ae3430 "Bug 1302573: [MSE] P2. Keep track of how much
> data can be evicted prior to current demuxing position. r=gerald"
> merging dom/media/mediasource/TrackBuffersManager.cpp
> merging dom/media/mediasource/TrackBuffersManager.h
> warning: conflicts while merging
> dom/media/mediasource/TrackBuffersManager.cpp! (edit, then use 'hg resolve
> --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

oh this was my bad, ignore this , sorry
Flags: needinfo?(jyavenard)
(Assignee)

Comment 46

2 years ago
So is it good or not. Maybe there's a dependency i forgot to uplift.

Updated

2 years ago
Blocks: 1323847

Updated

2 years ago
Blocks: 1324306
You need to log in before you can comment on or make changes to this bug.