Closed Bug 1302573 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1302573: [MSE] P1. Add mochitest.
58 bytes, text/x-review-board-request
mozbugz
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1302573: [MSE] P2. Keep track of how much data can be evicted prior to current demuxing position.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1302573: [MSE] P3. Display evictable amount in about:media output.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1302573: [MSE] P4. Be consistent in datatype being used.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1302573: [MSE] P5. Always evict data as soon as we can.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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.
See Also: → 1302465
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 ?
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.
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 on attachment 8793698 [details]
Bug 1302573: [MSE] P1. Add mochitest.

https://reviewboard.mozilla.org/r/80386/#review79050
Attachment #8793698 - Flags: review?(gsquelart) → 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 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 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+
Blocks: 1305284
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+
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
See Also: → 1305347
ni? myself to remember to request uplifting at some stage
Flags: needinfo?(jyavenard)
Can it be tested under recent nightly build ?
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
[Tracking Requested - why for this release]: This causes bug 1305284 and is a YouTube playback regression
tracking-firefox51: --- → ?
It is now behaving as expect.

Thank you.
\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)
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)
So is it good or not. Maybe there's a dependency i forgot to uplift.
Blocks: 1323847
Blocks: 1324306
You need to log in before you can comment on or make changes to this bug.