Closed Bug 1375772 Opened 7 years ago Closed 7 years ago

Another attempt to fix bug 1347174

Categories

(Core :: Audio/Video: Playback, enhancement, P2)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

(Keywords: stale-bug)

Attachments

(1 file, 2 obsolete files)

Bug 1375772 - don't evict the block which is in the current cached range.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
See bug 1347174 comment 10 for the root cause.

We have several bugs (some of them are in fact server side problems) revealed by the fix of bug 1347174S. To mitigate the issues, I will revert the change of bug 1347174S and fix the problem in another way.

Instead of changing the readahead limit, we will prevent readaehad blocks from being evicted in this bug since they will be used in the future as playback goes on.
Assignee: nobody → jwwang
Blocks: 1347174
Priority: -- → P1
Attachment #8880716 - Flags: review?(cpearce)
Attachment #8880717 - Flags: review?(cpearce)
Summary: Another attempt to fix bug 1347174S → Another attempt to fix bug 1347174
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #0)
> See bug 1347174 comment 10 for the root cause.
> 
> We have several bugs (some of them are in fact server side problems)
> revealed by the fix of bug 1347174S.

For the record, what are the bugs revealed?
Flags: needinfo?(jwwang)
Bug 1373618 comment 9.
Bug 1374199.
Flags: needinfo?(jwwang)
I'm not happy with the patches here as it makes us not evict overflow readahead data. This seems to result in the cache not being able to buffer after the playback position if the media cache was full and you seek. This UX makes me uncomfortable and makes Firefox seem to not do what it obviously should. So I debugged the underlying issue in bug 1373618, and put up a patch there to fix it.
Comment on attachment 8880716 [details]
Bug 1375772. P1 - don't evict readahead blocks since it could be expensive or impossible (for non-seekable streams) to download them later.

https://reviewboard.mozilla.org/r/152072/#review159212

I don't think we should do it this way; this results in unexpected behaviour, and doesn't fix the underlying issue. See my fix for the underlying issue in bug 1373618.
Attachment #8880716 - Flags: review?(cpearce) → review-
Comment on attachment 8880717 [details]
Bug 1375772. P2 - revert the changes in bug 1364001 and its followups.

https://reviewboard.mozilla.org/r/152074/#review159214
Attachment #8880717 - Flags: review?(cpearce) → review-
Sorry it took so long to look into this; it's basically impossible to focus enough to debug a complicated issue like this at an All Hands.
(In reply to Chris Pearce (:cpearce) from comment #5)
> I'm not happy with the patches here as it makes us not evict overflow
> readahead data.
We will evict PLAYED and METADATA blocks as playback goes on.

> This seems to result in the cache not being able to buffer
> after the playback position if the media cache was full and you seek.
1. We are still able to buffer after the playback position for we will evict PLAYED and METADATA blocks as playback goes on.
2. There is no problem in seeking either for we will evict blocks that are before the seek target for they become PLAYED after seeking.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #9)
> (In reply to Chris Pearce (:cpearce) from comment #5)
> > I'm not happy with the patches here as it makes us not evict overflow
> > readahead data.
> We will evict PLAYED and METADATA blocks as playback goes on.

Sure, but we don't seem to evict unplayed blocks before the playback position. So if the media cache fills up, and we seek into an unbuffered range, we'll not be able to fill the cache very much after the new playback position.

STR, with your patches here applied:
1. Find a URL to a video bigger than the media cache, or set the media cache to a smaller size to make it obvious.
2. Load the video, let the video buffer and until the media cache is full.
3. Once the buffered ranges stop expanding, seek to a position after the end of the last buffered range, and start playback.
4. Observe that after the seek the buffered range containing the playback position doesn't grow aggressively. I've observed on my machine that playback can even stutter a little, presumably because we're running the playback position very close to the end of buffered data.

Expected results:
After the seek to outside of the buffered ranges, I'd expect the newly created buffered range which contains the playback position to grow, and I'd expect data before the playback position to be evicted to facilitate this. Without your patch I see this, especially if I increase media.cache_readahead_limit and media.cache_resume_threshold.
My STR, with the patches applied:
1. go to about:config and change the value of "media.cache_size" to 10000.
2. open http://92.90.207.41/index.html (with MOZ_LOG=MediaCache:4 to dump MediaCache logs)
3. pause the video when currentTime is 2s and wait for download to stop.
4. seek to 2:15:51 and wait for download to stop.

For the logs I see:
1. Stream 0x7f1b5f46c258 Read at 3115873770 count=2582
2. Allocated block 129 to stream 0x7f1b5f46c258 block 95364(3124887552)

The last read position is 3115873770 and last write position is 3124887552 which is exactly ahead of 3115873770 by about 9MB. So the buffer grows as expected.

Note the video control doesn't show buffer ranges correctly. I have to deduce the ranges for the logs manually.

Can you provide the video link where the buffer doesn't grow as expected?
Flags: needinfo?(cpearce)
If you install the Media Panel add-on it will add a "Media" tab to the DevTools which will tell you the buffered ranges:
https://addons.mozilla.org/en-GB/firefox/addon/devtools-media-panel/
Thanks for the info.

I am able to the issue in comment 10 by seeking forward (to 2:00:00) and then seeking backward (to 5:00). Since the data around 2:00:00 are readahead blocks, they will never be evicted when the patches are applied. So you end up having only a little readahead data around the current playback position (5:00) which is bad.
Flags: needinfo?(cpearce)
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Attachment #8880716 - Attachment is obsolete: true
Attachment #8880717 - Attachment is obsolete: true
Attachment #8924563 - Flags: review?(cpearce)
Comment on attachment 8924563 [details]
Bug 1375772 - don't evict the block which is in the current cached range.

https://reviewboard.mozilla.org/r/195808/#review201704

Thanks!
Attachment #8924563 - Flags: review?(cpearce) → review+
Comment on attachment 8924563 [details]
Bug 1375772 - don't evict the block which is in the current cached range.

https://reviewboard.mozilla.org/r/195808/#review201708

::: commit-message-43a6b:1
(Diff revision 1)
> +Bug 1375772 - don't evit the block which is in the current cached range.

typo: don't evict
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fcb7efd9001
don't evict the block which is in the current cached range. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/3fcb7efd9001
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: