1185892 - Remove MediaDecoder::IsExpectingMoreData

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

As MediaDecoder::IsExpectingMoreData is all about asking MediaResource, we can create MediaResource::IsExpectingMoreData to replace it.

Comment 1

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

A step forward to achieve bug 1179498.

Comment 2

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d860d8de85dd

Comment 3

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

Attachment: MozReview Request: Bug 1185892. Part 1 - delegate the job of MediaSourceDecoder::IsExpectingMoreData to its MediaResource.

Bug 1185892. Part 1 - delegate the job of MediaSourceDecoder::IsExpectingMoreData to its MediaResource.

Comment 4

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

Attachment: MozReview Request: Bug 1185892. Part 2 - delegate the job of MediaDecoder::IsExpectingMoreData to its MediaResource.

Bug 1185892. Part 2 - delegate the job of MediaDecoder::IsExpectingMoreData to its MediaResource.

Comment 5

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

Attachment: MozReview Request: Bug 1185892. Part 3 - replace all calls to MediaDecoder::IsExpectingMoreData() with MediaResource::IsExpectingMoreData.

Bug 1185892. Part 3 - replace all calls to MediaDecoder::IsExpectingMoreData() with MediaResource::IsExpectingMoreData.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8636936 [details]
MozReview Request: Bug 1185892. Part 1 - delegate the job of MediaSourceDecoder::IsExpectingMoreData to its MediaResource.

https://reviewboard.mozilla.org/r/13763/#review12365

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8636937 [details]
MozReview Request: Bug 1185892. Part 2 - delegate the job of MediaDecoder::IsExpectingMoreData to its MediaResource.

https://reviewboard.mozilla.org/r/13765/#review12367

// MediaDecoder::mDecoderPosition is roughly the same as Tell() which
    // returns a position updated by latest Read() or ReadAt().
    return !IsDataCachedToEndOfResource(Tell()) && !IsSuspended();
  }
  
  I wonder how "rough" that is. Especially as Tell() is totally inaccurate, in particular with the MediaFormatReader() and the demuxer only using ReadAt() ; I'm guessing this will always return true with either the MP4Reader or the new WebMDemuxer even when we've cached the entire file.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8636938 [details]
MozReview Request: Bug 1185892. Part 3 - replace all calls to MediaDecoder::IsExpectingMoreData() with MediaResource::IsExpectingMoreData.

https://reviewboard.mozilla.org/r/13767/#review12369

Ship It!

Comment 9

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

https://reviewboard.mozilla.org/r/13765/#review12367

https://hg.mozilla.org/mozilla-central/file/8e5c888d0d89/dom/media/MediaDecoder.cpp#l989
|mDecoderPosition = aOffset + aBytes| which is exactly the stream positoin after Read() or ReadAt().

In fact, Tell() is the most updated version of mDecoderPosition since each Read() or ReadAt() will dispatch an async update to mDecoderPosition.

It is also true that MediaFormatReader() using ReadAt() will also trigger updates of mDecoderPosition to make it less accurate even without this bug. I think we can tell the MediaResource not to dispatch the update when it is a silent read to fix the problem.

Comment 10

[Jean-Yves Avenard [:jya]]

SilentReadAt() is only used with older DecoderReader, the one relying on NotifyDataArrived() to update their buffered range (I believe this is limited to WebMReader, MP3Reader and the Ogg reader)

No MediaFormatDemuxer should be using it ..

But you're right, not dispatching the update with SilentReadAt() would fix it for those; though, does reading from the cache also trigger the updates ? SilentReadAt should only ever read from the cache now

Comment 11

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

https://hg.mozilla.org/mozilla-central/file/8e5c888d0d89/dom/media/MediaResource.cpp#l736

Only Read() or ReadAt() will trigger the update. ReadFromCache() doesn't have the side effect.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f09e5cfeac1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aca86d37171
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d8399fa276

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f09e5cfeac1
https://hg.mozilla.org/mozilla-central/rev/5aca86d37171
https://hg.mozilla.org/mozilla-central/rev/e6d8399fa276