Closed Bug 1190238 Opened 4 years ago Closed 4 years ago

Remove MediaResource::Read/Seek/Tell API

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 + fixed
firefox42 + fixed
firefox43 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → jyavenard
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.
Attachment #8647257 - Flags: review?(cpearce)
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.
Attachment #8647258 - Flags: review?(cpearce)
This allows to remove a fair amount of duplicated logic.
Most of it is in obsoleted code though.
Attachment #8647259 - Flags: review?(cpearce)
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.
Attachment #8647271 - Flags: review?(cpearce)
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.
Attachment #8647272 - Flags: review?(cpearce)
Blocks: 1191813
Attachment #8647257 - Attachment is obsolete: true
Attachment #8647257 - Flags: review?(cpearce)
Attachment #8647258 - Attachment is obsolete: true
Attachment #8647258 - Flags: review?(cpearce)
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.
Attachment #8647271 - Flags: review?(cpearce) → review+
Attachment #8647272 - Flags: review?(cpearce) → review+
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.
Attachment #8647259 - Flags: review?(cpearce) → review+
[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.
Blocks: 1194884
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
Attachment #8647271 - Flags: approval-mozilla-beta?
Attachment #8647271 - Flags: approval-mozilla-aurora?
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
Attachment #8647272 - Flags: approval-mozilla-beta?
Attachment #8647272 - Flags: approval-mozilla-aurora?
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.
Attachment #8647271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8647272 [details] [diff] [review]
P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

Aurora+
Attachment #8647272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracked because it's a regression and adding the keyword regression based on comment #12.
Keywords: regression
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.
Attachment #8647271 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8647272 [details] [diff] [review]
P2. Have MediaResourceIndex::ReadAt() only stop early when reaching EOS or error.

Beta+
Attachment #8647272 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm hitting conflicts trying to uplift this to beta.
Can you attach a branch-specific patch for this?
Flags: needinfo?(jyavenard)
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
Flags: needinfo?(jyavenard)
Blocks: 1196591
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
See Also: → 1205209
Depends on: 1256038
No longer depends on: 1256038
You need to log in before you can comment on or make changes to this bug.