915957 - Call MediaDecoder::NotifyBytesConsumed() from MediaResource::Read functions

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Call MediaDecoder::NotifyBytesConsumed from MediaResource::Read functions

In every MediaDecoderReader backend we have to remember to call MediaDecoder::NotifyBytesConsumed() after every call to MediaResource::Read(). This is silly, we can just have MediaResource::Read() and friends call this for us, then we wouldn't forget to call this in the readers.

We also forgot to call NotifyBytesConsumed() in our B2G and Android backends, we won't need to worry about that now.

Green with attached patch:
https://tbpl.mozilla.org/?tree=Try&rev=e9df2df1e3e6

Comment 1

[Matthew Gregan [:kinetik]]

Comment on attachment 804156 [details] [diff] [review]
Patch: Call MediaDecoder::NotifyBytesConsumed from MediaResource::Read functions

Review of attachment 804156 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the ReadAt issue resolved.

::: content/media/MediaResource.cpp
@@ +818,5 @@
>    NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");
>  
> +  nsresult rv = mCacheStream.ReadAt(aOffset, aBuffer, aCount, aBytes);
> +  if (NS_SUCCEEDED(rv)) {
> +    DispatchBytesConsumed(*aBytes);

I don't think we can do this for ReadAt because of the explicit offset parameter unless you save the highest reported offset and only report reads that extend that value.  If the caller did ReadAt(0, ..., 1024, ...) three times, we'd report 3072 bytes consumed despite logically having consumed only 1024 from the stream.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Matthew Gregan [:kinetik] from comment #1)
> I don't think we can do this for ReadAt because of the explicit offset
> parameter unless you save the highest reported offset and only report reads
> that extend that value.  If the caller did ReadAt(0, ..., 1024, ...) three
> times, we'd report 3072 bytes consumed despite logically having consumed
> only 1024 from the stream.

Hmm, but then if we seeked to the end of stream, say to get an index, then we'd stop reporting any reads.

How about:
1. We have MediaDecoder::NotifyBytesConsumed() ignore the notification if mIgnoreProgressData==false (so we don't send notifications when we're reading metadata and seeking, which is when we don't have a valid mDecoderPosition field).
2. We add an offset parameter to MediaDecoder::NotifyBytesConsumed(), to denote the start offset where the data was read. MediaDecoder::NotifyBytesConsumed() can then keep track of whether the read appends to mDecoderPosition and ignores the notification for offsets < mDecoderPosition. For offsets >= mDecoderPosition, we set mDecoderPosition to offset+aBytesConsumed (the end of the range read).

Comment 3

[Matthew Gregan [:kinetik]]

Sounds good.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #2)
> (In reply to Matthew Gregan [:kinetik] from comment #1)
> > I don't think we can do this for ReadAt because of the explicit offset
> > parameter unless you save the highest reported offset and only report reads
> > that extend that value.  If the caller did ReadAt(0, ..., 1024, ...) three
> > times, we'd report 3072 bytes consumed despite logically having consumed
> > only 1024 from the stream.
> 
> Hmm, but then if we seeked to the end of stream, say to get an index, then
> we'd stop reporting any reads.
> 
> How about:
> 1. We have MediaDecoder::NotifyBytesConsumed() ignore the notification if
> mIgnoreProgressData==false (so we don't send notifications when we're
> reading metadata and seeking, which is when we don't have a valid
> mDecoderPosition field).


Urgh, I mean ignore the notification if mIgnoreProgressData == *true* of course.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2: Call MediaDecoder::NotifyBytesConsumed from MediaResource::Read functions

With the above discussed changes.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Greenish on try:
https://tbpl.mozilla.org/?tree=Try&rev=d8f596e0a52d

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10065efeed2f

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10065efeed2f