Closed Bug 1093880 Opened 5 years ago Closed 5 years ago

TrackBuffer::InitializeDecoder may fail or block

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file, 2 obsolete files)

There is a potential race condition in TrackBuffer::AppendData

TrackBuffer::AppendData creates a new decoder and queue it to be initialized in another task and then appends data to the current resource.
The initialization of the decoder need the data that has been appended to the resource.

It is possible for the initialization task to run before TrackBuffer::AppendData completes, at which time the data required for the initialization isn't yet available.

This would cause ReadMetadata to either fail (mp4) or stall (webm).

The initialization task should only be queued once TrackBuffer::AppendData has completed.
Delay initialization of decoder until data has been appended. This allows to pass the mochitests and prevent timing out. There is still the possibility of timeout should we create the decoder while the media task queue is flushing. We should abort early and fail. This will be the subject of another bug
Attachment #8517034 - Flags: review?(cajbir.bugzilla)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Summary: TrackBuffer → TrackBuffer::InitializeDecoder may fail or block
Comment on attachment 8517034 [details] [diff] [review]
Delay decoder initialization until data has been appended

Failed to build on B2G
Attachment #8517034 - Attachment is obsolete: true
Attachment #8517034 - Flags: review?(cajbir.bugzilla)
Fix B2G compilation
Attachment #8517100 - Flags: review?(cajbir.bugzilla)
Blocks: 1093927
Comment on attachment 8517100 [details] [diff] [review]
Delay decoder initialization until data has been appended

I usually think EIBTI, but this works.

>+class DecodersToInitialize {

Please add MOZ_STACK_CLASS, for the sake of the mOwner pointer.

>+  DecodersToInitialize(TrackBuffer* aOwner)

explicit, just in case.

>+  nsTArray<nsRefPtr<SourceBufferDecoder>> mDecoders;

Currently there should be only one decoder, but, when we follow the spec
better, we can expect to need more occasionally.

Please use nsAutoTArray<nsRefPtr<SourceBufferDecoder>,N> to save allocation in
the common (and currently only) case.

>+nsRefPtr<SourceBufferDecoder>
> TrackBuffer::NewDecoder()

Convention is to return already_AddRefed<SourceBufferDecoder>.  Sticking with
conventions makes patterns easier to recognize, and this one may avoid some
unnecessary thread-safe AddRef/Release pairs.  RVO may or may not eliminate
them in use cases here, and would not if the return value were used in
assignment instead of initialization.

>   if (!decoder) {
>-    return false;
>+    return decoder;

return nullptr, for clarity.

>+  return decoder;

return decoder.forget() for already_AddRefed.

>+  // The decoder is not considered initialized until it is added to mDecoders.

The new decoder is added to mDecoders immediately, so this sentence is
incorrect.  Perhaps you mean mInitializedDecoders?
Attachment #8517100 - Flags: review?(cajbir.bugzilla) → review+
(In reply to Karl Tomlinson (:karlt) from comment #5)
> Comment on attachment 8517100 [details] [diff] [review]
> Delay decoder initialization until data has been appended
> 
> I usually think EIBTI, but this works.
> 

Had to google that one !
still not sure what you mean in this context


> Currently there should be only one decoder, but, when we follow the spec
> better, we can expect to need more occasionally.

the code as it is, would allow up to two decoders created. As I have no idea on how this is called, I kept it vague enough for it.

> 
> >+  // The decoder is not considered initialized until it is added to mDecoders.
> 
> The new decoder is added to mDecoders immediately, so this sentence is
> incorrect.  Perhaps you mean mInitializedDecoders?

this is the original comment, the ultimate behaviour being unchanged, I gather that the original comment was wrong :)

thanks for the review
Attachment #8517100 - Attachment is obsolete: true
Keywords: checkin-needed
Replying just to explain.

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> (In reply to Karl Tomlinson (:karlt) from comment #5)
> > I usually think EIBTI, but this works.
> 
> Had to google that one !
> still not sure what you mean in this context

It is not clear from looking just at AppendBuffer that QueueInitializeDecoder
is called.

RAII classes make sense when destruction is just resource release, but when
destruction is also queue another task, that is hiding a bit more from the
reader.

I probably would have managed the array in AppendBuffer().

There are some alternate returns, but I don't think any can occur after
successful NewDecoder, though I agree that is not clear from the immediately
surrounding code.

At some stage this code will be a loop I expect, to handle multiple init segments, and some early returns can be break statements.

> > Currently there should be only one decoder, but, when we follow the spec
> > better, we can expect to need more occasionally.
> 
> the code as it is, would allow up to two decoders created. As I have no idea
> on how this is called, I kept it vague enough for it.

Yes, it depends on the implementation of IsMediaSegmentPresent().

> this is the original comment, the ultimate behaviour being unchanged, I
> gather that the original comment was wrong :)

Ah right :)
https://hg.mozilla.org/mozilla-central/rev/06275a674a57
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.