Closed Bug 1361964 Opened 3 years ago Closed 3 years ago

WebMDemuxer reads at the end of the cache stream from time to time

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

(Keywords: testcase-wanted)

Attachments

(1 file, 1 obsolete file)

See bug 1347174 comment 22 for the discussion.

This causes media.cache_readahead_limit fail to work as expected.
See Also: → 1347174
WebMBufferParser only attempts to read the block reported by MediaResource:GetCachedRanges()

it never attempts to read outside those block except to demux the next block (in which case it only reads what it needs)
Attachment #8864537 - Flags: review?(jyavenard)
Attachment #8864538 - Flags: review?(jyavenard)
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139276

I don't see why need this change...

It seems to be a workaround for something else broken.

When we retrieve the buffered cache range using MediaResource::GetCachedRanges, we first use an AutoPinned<MediaResource> that is supposed to guarantee that no data will get evicted from the MediaCache.

That is, a call to ReadFromCache will always succeed and will never need to download new data.

What the problem reported indicates is that AutoPinned<MediaResource> is broken and can in fact cause more data to be eviced, that or GetCachedRanges return the wrong value.

As such, modification of ReadFromCache isn't desirable. Better fix the real problem that work around it.
Attachment #8864537 - Flags: review?(jyavenard)
Comment on attachment 8864538 [details]
Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

https://reviewboard.mozilla.org/r/136214/#review139282

Same as in P1.

WebMBufferedState::UpdateIndex is called while the MediaResource is pinned, and is only called with byte range returned by MediaResource::GetCachedRanges.

As such, MediaResource::MediaReadAt will only ever be called with a byte range known to be cached.
Attachment #8864538 - Flags: review?(jyavenard) → review-
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139276

See https://bugzilla.mozilla.org/show_bug.cgi?id=1347174#c22 for the detail.
MediaCache will need to download new data not because we read beyond the cached ranges, but because the readahead feature. MediaCacheStream::Read() will change mStreamOffset which affects the calculation of readahead. Therefore, we should call MediaCacheStream::ReadFromCache() instead which doesn't change mStreamOffset.

See http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/dom/media/MediaCache.cpp#1244 for how readahead is calculated.
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139276

To me the right approach is not have ReadFromCache use readahead, and that should be disabled... We only want to read from the cache and nothing else as per the function name and this is also how it's used.
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139276

ReadFromCache is also always supposed to succeed. It's an error to attempt to read more than what is cached.
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139528

::: dom/media/BufferMediaResource.h:91
(Diff revision 1)
>    {
>      if (aOffset < 0) {
>        return NS_ERROR_FAILURE;
>      }
>  
> -    uint32_t bytes = std::min(mLength - static_cast<uint32_t>(aOffset), aCount);
> +    *aBytes = std::min(mLength - static_cast<uint32_t>(aOffset), aCount);

we should instead return an error if we attempt to read more than what's there to read.

After all, ReadFromCache should never attempt to read passed the end of the resource.
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139528

> we should instead return an error if we attempt to read more than what's there to read.
> 
> After all, ReadFromCache should never attempt to read passed the end of the resource.

http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/dom/media/MediaCache.h#307
The comment describes as you said. However the implementaton could return SUCCESS with fewer bytes read.

Anyway, I will remove P1 and assume ReadFromCache() should only succeed with full bytes read.
Comment on attachment 8864537 [details]
Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

https://reviewboard.mozilla.org/r/136212/#review139528

> http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/dom/media/MediaCache.h#307
> The comment describes as you said. However the implementaton could return SUCCESS with fewer bytes read.
> 
> Anyway, I will remove P1 and assume ReadFromCache() should only succeed with full bytes read.

I think I was wrong. The implementation does follow the comment and only return SUCCESS when full bytes are read.
Attachment #8864537 - Attachment is obsolete: true
Comment on attachment 8864538 [details]
Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

https://reviewboard.mozilla.org/r/136214/#review139548
Attachment #8864538 - Flags: review?(jyavenard) → review+
Thanks!
Assignee: nobody → jwwang
Blocks: 1347174
Priority: -- → P1
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7689c6b41df
WebMBufferedState::UpdateIndex() should read from cache. r=jya
We should add a test case to ensure the pref "media.cache_readahead_limit" is respected.

Hi Blake,
Can you put this bug in your plate and find someone to write a test case for it? Thanks!
Flags: needinfo?(bwu)
Keywords: testcase-wanted
https://hg.mozilla.org/mozilla-central/rev/c7689c6b41df
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #16)
> We should add a test case to ensure the pref "media.cache_readahead_limit"
> is respected.
> 
> Hi Blake,
> Can you put this bug in your plate and find someone to write a test case for
> it? Thanks!
Sure. You have marked it as testcase-wanted. Sweet.
Flags: needinfo?(bwu)
Comment on attachment 8864538 [details]
Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

Approval Request Comment
[Feature/Bug causing the regression]:unknown
[User impact if declined]: ore data will be downloaded than necessary to waste the bandwidth of the user when playing WebM files.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low.
[Why is the change risky/not risky?]: The change is simple and I've test it manually.
[String changes made/needed]: None.
Attachment #8864538 - Flags: approval-mozilla-beta?
Comment on attachment 8864538 [details]
Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

See comment #34, I would like it to bake more on nightly. Beta54-.
Attachment #8864538 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.