1191813 - MP3 playback skips to the end

Status: RESOLVED FIXED

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

Description

[Eugen Sawin [:esawin]]

This is an intermittent issue on Android that occurs mostly during the beginning of playback, shortly after the initialization of the demuxer. The playback skips to the end of the resource and stops.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1191813-Add-blocking-read-to-ensure-complete-fra.patch

The issue is caused by incomplete frame reads.

After finding the next frame header, we call GetNextFrame to read the complete frame using MediaResource::ReadAt. However, this is not guaranteed to give us the complete requested buffer range, in which case we abort the GetSamples call with an END_OF_STREAM error.

This can also happen during initialization, in which case we reject with a WAITING_FOR_DATA error.

The patch introduces a blocking read which tries to get the complete frame by repeated calls to MediaResource::ReadAt to avoid premature exits.

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8644370 [details] [diff] [review]
0001-Bug-1191813-Add-blocking-read-to-ensure-complete-fra.patch

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

Why not let MediaResource::ReadAt block rather than reimplementing a blocking read on top of a non-blocking read on top of a blocking read?

I also don't understand the purpose of the retry counter.  With a normal blocking read, the caller expects the complete read (or EOF) or a failure.  Introducing a new state where it can fail after a number of retries but would later succeed if more data became available seems unusual for a blocking read call.

Comment 3

[Eugen Sawin [:esawin]]

> Why not let MediaResource::ReadAt block rather than reimplementing a
> blocking read on top of a non-blocking read on top of a blocking read?

The way I understand it, MediaResource::ReadAt, or more specifically MediaCacheStream::Read is not meant to be blocking, which is desired in most cases (including the case of MP3TrackDemuxer::FindNextFrame).

Having a blocking wrapper function around it seems like a special use case and was usually implemented by the resource consumer so far (e.g., MP4Stream::BlockingReadIntoCache).

We could provide MediaResource::BlockingReadAt, if that's something we want to generalize.

> I also don't understand the purpose of the retry counter.  With a normal
> blocking read, the caller expects the complete read (or EOF) or a failure. 
> Introducing a new state where it can fail after a number of retries but
> would later succeed if more data became available seems unusual for a
> blocking read call.

Sorry about that, that's not meant to be there. I'll update the patch with the proper way of exiting the loop on error. We don't need to retry after receiving a zero-byte range or an error.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1191813-Add-blocking-read-to-ensure-complete-fra.patch

Removed retries on failed read, zero-byte range read will exit the blocking read now.

Comment 5

[Eugen Sawin [:esawin]]

Correction for comment 3: MediaCacheStream::Read blocks if no cached blocks are available for the requested range, so it's not entirely non-blocking.

Comment 6

[Eugen Sawin [:esawin]]

Will anything change regarding blocking reads with bug 1190238 that could help with this bug?

Comment 7

[Jean-Yves Avenard [:jya]]

I don't see why ReadAt wouldn't be blocking or be blocking: I see nothing that says it's one behaviour or the other: it really depends on the type of resources. It will return 0 if EOS is reached ; so looping until it returns 0 is fine.

You could instead use MediaResource::MediaReadAt which will do exactly that for you.

The only issue we had was if we attempted to read past the length of the MediaResource which is only an issue with MSE as the SourceBufferResource will block.

With the new MSE architecture, we could change SourceBufferResource so it doesn't block when reaching the end, and instead returns immediately. This would remove the requirement of never attempting to read past the resource's length.

Does that answer your question?

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8644921 [details] [diff] [review]
0001-Bug-1191813-Add-blocking-read-to-ensure-complete-fra.patch

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

sounds overly complicated ; and you have convenience method available that could simplify all of this.

Could either implement that now, or revisit later ; up to you.


I'm guessing you'll want this uplifted to aurora (and even beta)

::: dom/media/MP3Demuxer.cpp
@@ +538,5 @@
> +    readTotal += read;
> +
> +    if (!read) {
> +      // The media resource is guaranteed to return a cached block or force
> +      // a blocking read, we should never receive a zero-byte range.

well, you can receive 0 byte range ; just means you've reached EOS

@@ +550,5 @@
> +MP3TrackDemuxer::Read(uint8_t* aBuffer, int64_t aOffset, int32_t aSize) {
> +  MP3DEMUXER_LOGV("MP3TrackDemuxer::Read(%p %" PRId64 " %d)",
> +                  aBuffer, aOffset, aSize);
> +
> +  aSize = ClampReadSize(aOffset, aSize);

you could use MediaReadAt.

And the MediaResourceIndex I'm currently working on would also easily integrate with all this.

Comment 9

[Eugen Sawin [:esawin]]

> sounds overly complicated ; and you have convenience method available that
> could simplify all of this.

MediaReadAt was not available at the time of the original implementation, but it looks like it would simplify the code a little, so I'm going with that.

> I'm guessing you'll want this uplifted to aurora (and even beta)

Yes, I would like to uplift this to 41 eventually.

> And the MediaResourceIndex I'm currently working on would also easily
> integrate with all this.

Can you please provide the bug number, it sounds interesting if the plan is to uplift it to 41.

Is there a reason to have inconsistent interfaces between ReadAt and MediaReadAt (the latter allocates its own buffer). Since the MP3 demuxer reads frames individually, it looks like it would cause a lot of wasteful dynamic micro-allocations.

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #9)
> > sounds overly complicated ; and you have convenience method available that
> > could simplify all of this.
> 
> MediaReadAt was not available at the time of the original implementation,
> but it looks like it would simplify the code a little, so I'm going with
> that.
> 
> > I'm guessing you'll want this uplifted to aurora (and even beta)
> 
> Yes, I would like to uplift this to 41 eventually.
> 
> > And the MediaResourceIndex I'm currently working on would also easily
> > integrate with all this.
> 
> Can you please provide the bug number, it sounds interesting if the plan is
> to uplift it to 41.

bug 1190238


> 
> Is there a reason to have inconsistent interfaces between ReadAt and
> MediaReadAt (the latter allocates its own buffer). Since the MP3 demuxer
> reads frames individually, it looks like it would cause a lot of wasteful
> dynamic micro-allocations.

Legacy I guess.

Returning an allocated rec-counted buffer is very useful in some places where you don't know how much data you're going to end up with.

I could make the MediaResourceIndex::ReadAt (from bug 1190238) provides a similar behaviour as MediaReadAt ; especially as it appears that most places using MediaReadAt loop themselves until it returns an error or *aByte is set to 0.

That is, it will always read all the data you've asked for until you reach EOS.

Otherwise, yes, for the MP3 Demuxer after looking at it, returning a newly allocated buffer wouldn't be super efficient.

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1191813-Add-buffer-allocating-wrapper-for-ReadAt.patch

It looks like there are two different patterns mixed into the MediaRead implementation. MediaRead itself provides the guarantee of the full byte range read up to EOS on top of ReadAt. Additionally, the implementation is wrapped into a convenience function which allocates a buffer.

Ideally, we should provide both patterns separately and keep the basic API open for all types of use cases.

I would like to find a good solution for this, so I'm looking for some feedback.

The first (this) patch provides the convenient buffer allocation wrapper.

The second patch splits the MediaRead into the basic API and helper functions to reduce the existing and future code duplication. The reason to use a function template for this is because MediaCacheStream does not share a common base with MediaResource. Alternatively, we could change that instead and provide a "ReadableBase".

Comment 12

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1191813-Generalize-MediaReadAt-to-allow-for-pre-.patch

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1191813-Add-blocking-read-to-ensure-complete-fra.patch

This is how the blocking read would look in the MP3 demuxer based on the changed API.

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647130 [details] [diff] [review]
0002-Bug-1191813-Generalize-MediaReadAt-to-allow-for-pre-.patch

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

I've already added a method in bug 1190238.

And that makes all user of ReadAt benefit from it

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647129 [details] [diff] [review]
0001-Bug-1191813-Add-buffer-allocating-wrapper-for-ReadAt.patch

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

I don't see much use of this function outside what you want to do in the MP3 user.

I think it also blurs who does what ..

I also think that passing function ptr as a template parameter makes it super hard to read the code.

Comment 16

[Jean-Yves Avenard [:jya]]

With the MediaResourceIndex class ; you would instead do MediaResourceIndex resource(mResource);
resource.ReadAt(....)

This will read as much as what you ask unless an error or EOS is reached.

Returning when an EOS is encountered is unfortunately, not a behaviour that one can assume to be reliable. Some MediaResource *will* block waiting for more in which case you will deadlock. 

And checking the length of the mediaresource isn't something you want to generalise either, because that value can change over time, or even within a call to ReadAt ; and some mediaresource client are relying on this.

So checking the length is still the way to go before calling ReadAt

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: mp3demuxerblocking.patch

This is all you would need with bug 1190238 in I think...

Comment 18

[Eugen Sawin [:esawin]]

Comment on attachment 8647266 [details] [diff] [review]
mp3demuxerblocking.patch

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

This looks great, assuming you plan to uplift the patches from bug 1190238 to 41.

Comment 19

[Jean-Yves Avenard [:jya]]

Yes, I'm going to let it bake in central for a week ; and then request uplift.

A question: why did you only use the "BlockingRead" in GetNextFrame.

I think it should be done everywhere.

It's not really a blocking read btw, it's more like a read until you can't .

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647266 [details] [diff] [review]
mp3demuxerblocking.patch

Will work one getting 1190238 uplifted to beta.

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Eugen - what Firefox versions should we request tracking on for this bug?

Comment 22

[Eugen Sawin [:esawin]]

> Yes, I'm going to let it bake in central for a week ; and then request
> uplift.

That's great!

> A question: why did you only use the "BlockingRead" in GetNextFrame.
> 
> I think it should be done everywhere.

Only GetNextFrame knows the exact size and offsets of the frame, reading in FindNextFrame can remain "greedy" as we're just searching for the next frame header there and during initialization we don't know its offsets. So using a "blocking read" there is optional.

There are some optimization options we could apply here since we do have a good idea about the next frame header size and position once we've parsed the previous frame, so switching to the "blocking read" there would help us long-term, too.

Comment 23

[Jean-Yves Avenard [:jya]]

the thing is...

this is not a "blocking" read ; MediaResource::ReadAt may return less than what you're asking for, if for example you're reading at a boundary of cached data.

The first ReadAt will return the cache data only, the 2nd ReadAt returns the remaining piece.

So calling ReadAt multiple times until you get an error or 0 is the proper things to do.

Comment 24

[Jean-Yves Avenard [:jya]]

or until you get the number of bytes you first asked for of course

Comment 25

[Eugen Sawin [:esawin]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #21)
> Eugen - what Firefox versions should we request tracking on for this bug?

As long as bug 1190238 is safe to uplift, I would like to track this one for 41. This bug and bug 1190558 were the most frequent issues I have encountered during testing.

Comment 26

[Eugen Sawin [:esawin]]

> the thing is...
> 
> this is not a "blocking" read ; MediaResource::ReadAt may return less than
> what you're asking for, if for example you're reading at a boundary of
> cached data.
> 
> The first ReadAt will return the cache data only, the 2nd ReadAt returns the
> remaining piece.

Yes, that's what's causing the issue in GetNextFrame.

> So calling ReadAt multiple times until you get an error or 0 is the proper
> things to do.

FindNextFrame doesn't know the actual size it needs to read yet, it just reads along until it finds a frame header, so it's not an issue there. ReadAt will not return a 0-size byte range before EOS, which means FindNextFrame will parse what it gets (the cached line) and then request the next block, if it doesn't find a header in the cached range.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d780f3b4f9f7

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647266 [details] [diff] [review]
mp3demuxerblocking.patch

Approval Request Comment
[Feature/regressing bug #]: 1166779
[User impact if declined]: Mp3 audio playback will stop early
[Describe test coverage new/current, TreeHerder]: In central for over a week
[Risks and why]: None. This is making use of the new functionality added in bug 1190238 which has already been backported to aurora and beta.
[String/UUID change made/needed]: None

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647266 [details] [diff] [review]
mp3demuxerblocking.patch

Potential important regession, taking it.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7da395d1e12

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/25548a16090a