Closed Bug 1185892 Opened 4 years ago Closed 4 years ago

Remove MediaDecoder::IsExpectingMoreData

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

As MediaDecoder::IsExpectingMoreData is all about asking MediaResource, we can create MediaResource::IsExpectingMoreData to replace it.
A step forward to achieve bug 1179498.
Blocks: 1179498
Bug 1185892. Part 1 - delegate the job of MediaSourceDecoder::IsExpectingMoreData to its MediaResource.
Attachment #8636936 - Flags: review?(jyavenard)
Bug 1185892. Part 2 - delegate the job of MediaDecoder::IsExpectingMoreData to its MediaResource.
Attachment #8636937 - Flags: review?(jyavenard)
Bug 1185892. Part 3 - replace all calls to MediaDecoder::IsExpectingMoreData() with MediaResource::IsExpectingMoreData.
Attachment #8636938 - Flags: review?(jyavenard)
Assignee: nobody → jwwang
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
Attachment #8636936 - Flags: review?(jyavenard) → review+
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.
Attachment #8636937 - Flags: review?(jyavenard) → review+
Attachment #8636938 - Flags: review?(jyavenard) → review+
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!
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.
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
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.
You need to log in before you can comment on or make changes to this bug.