1190238 - Remove MediaResource::Read/Seek/Tell API

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

MediaResource provides Seek/Read/Tell API .
But those could be improperly used, aren't thread-safe as one read would cause an invalid thread to read the wrong value later etc.

The incorrect use of those APIs cause problems with a few MediaDecoderReader , in particular the WebMReader, MP3Reader, GStreamReader, OggReader.

Instead it should use a wrapping class, that will provide such functions and will not have side-effects like the current MediaResource does.

It will allow to remove MediaResource::SilentReadAt which has introduced serious performance regression issue.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove MediaResource::Read/Seek.

This functionality is now replaced with a dedicated new MediaResourceIndex class.
This allows for concurrent Read/Seek use of the MediaResource without having side effects.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

MediaResource::ReadAt() requires to loop several times to ensure that all data has been read as it may return less data than requested.
This will allow to remove the handling of this particular shortcoming in MediaResources' users.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P3. Do not loop calling MediaResource::Read or ReadAt, let MediaResourceIndex do it for us.

This allows to remove a fair amount of duplicated logic.
Most of it is in obsoleted code though.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove MediaResource::Read/Seek v2.

This functionality is now replaced with a dedicated new MediaResourceIndex class.
This allows for concurrent Read/Seek use of the MediaResource without having side effects.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

MediaResource::ReadAt() requires to loop several times to ensure that all data has been read as it may return less data than requested.
This will allow to remove the handling of this particular shortcoming in MediaResources' users.

Comment 6

[Jean-Yves Avenard [:jya]]

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

Comment 7

[Jean-Yves Avenard [:jya]]

Fixed null deref:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb5c6e409cc

Comment 8

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

Comment on attachment 8647271 [details] [diff] [review]
P1. Remove MediaResource::Read/Seek v2.

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

Nice.

::: dom/media/MediaResource.h
@@ +790,5 @@
> +    : mResource(aResource)
> +    , mOffset(0)
> +  {}
> +
> +    // Read up to aCount bytes from the stream. The buffer must have

Indentation is off here.

Comment 9

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

Comment on attachment 8647259 [details] [diff] [review]
P3. Do not loop calling MediaResource::Read or ReadAt, let MediaResourceIndex do it for us.

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

::: dom/media/apple/AppleMP3Reader.cpp
@@ +100,2 @@
>  
>    // We will have read some data in the last iteration iff we filled the buffer.

This comment here is now obsolete too; it mentions a loop that you removed.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4f1b9b7c01
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ba1b327fc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/66eba222c767

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba4f1b9b7c01
https://hg.mozilla.org/mozilla-central/rev/d8ba1b327fc0
https://hg.mozilla.org/mozilla-central/rev/66eba222c767

Comment 12

[Jean-Yves Avenard [:jya]]

[Tracking Requested - why for this release]: In 41 ; the following changes https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=28e6664ad1a1&tochange=9fb9dbd7a0dd
introduced a wild range of regressions ; going from invalid duration reported, media playback being interrupted, high CPU usage or even crashes (gstreamer).

This bug fixed the root cause and should be uplifted to 41. I will let it bake in central for a week or so and then apply for uplift.

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647271 [details] [diff] [review]
P1. Remove MediaResource::Read/Seek v2.

Approval Request Comment
[Feature/regressing bug #]: bug 1175768
[User impact if declined]: High CPU usage during playback, playback crashing or failing (in particular linux)
[Describe test coverage new/current, TreeHerder]: In central for a week, local tests. Multiple reports that it made everything working okay again.
[Risks and why]: Low. While this is an extensive patch set, the logic is very sound and has been extensively tested.
[String/UUID change made/needed]: None

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647272 [details] [diff] [review]
P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

With optional request for part 3 (which adds very low risk)

Approval Request Comment
[Feature/regressing bug #]: bug 1175768
[User impact if declined]: Inability to uplift other regression fixes that rely on this change.
[Describe test coverage new/current, TreeHerder]: In central for a week, local tests.
[Risks and why]: Very low. It just moves what all callers of ReadAt were doing in a centralized place
[String/UUID change made/needed]: None

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8647271 [details] [diff] [review]
P1. Remove MediaResource::Read/Seek v2.

Given that this fix has baked on Nightly for a few days, it's safe to uplift to Aurora. We can uplift to Beta a day or two after that. This is a pretty big code change, it seems best to stabilize over days and across channels.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8647272 [details] [diff] [review]
P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

Aurora+

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked because it's a regression and adding the keyword regression based on comment #12.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06da1c87b289
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e34ca12b2ef
https://hg.mozilla.org/releases/mozilla-aurora/rev/b6a87b673748

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8647271 [details] [diff] [review]
P1. Remove MediaResource::Read/Seek v2.

Aurora uplift 2 days ago hasn't uncovered any issues, safe to uplift to Beta.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8647272 [details] [diff] [review]
P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

Beta+

Comment 21

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

I'm hitting conflicts trying to uplift this to beta.
Can you attach a branch-specific patch for this?

Comment 22

[Jean-Yves Avenard [:jya]]

conflicts appeared worse than what they really were.

It doesn't build locally anymore for some reason, so going through try first:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed837d7d5679

Comment 23

[Jean-Yves Avenard [:jya]]

weird R4 error on this try run on linux
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cfeeb7bbd9

as I can't see how there could be any link, running a try run with plain beta:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9b3bdc0e224

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6d34aa009ece
https://hg.mozilla.org/releases/mozilla-beta/rev/e6cb2350e5d8
https://hg.mozilla.org/releases/mozilla-beta/rev/d0333171dad9