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

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

See bug 1347174 comment 22 for the discussion.

This causes media.cache_readahead_limit fail to work as expected.

Comment 1

[Jean-Yves Avenard [:jya]]

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)

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1361964. P1 - let ReadFromCache() return the number of bytes read successfully.

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

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

We don't want to trigger download when calculating buffer ranges since download
changes buffer ranges.

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

Comment 4

[Jean-Yves Avenard [:jya]]

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.

Comment 5

[Jean-Yves Avenard [:jya]]

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.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

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 7

[Jean-Yves Avenard [:jya]]

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 8

[Jean-Yves Avenard [:jya]]

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 9

[Jean-Yves Avenard [:jya]]

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 10

[JW Wang [:jwwang] [:jw_wang]]

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 11

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8864538 [details]
Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

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

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8864538 [details]
Bug 1361964 - WebMBufferedState::UpdateIndex() should read from cache.

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

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7689c6b41df
WebMBufferedState::UpdateIndex() should read from cache. r=jya

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

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!

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Blake Wu [:bwu][:blakewu]]

(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.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 20

[Gerry Chang [:gchang]]

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-.