Unnecessary task dispatch in DecoderCallbackFuzzingWrapper::ReleaseMediaResources()

RESOLVED INVALID

Status

()

Core
Audio/Video: Playback
RESOLVED INVALID
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

2 years ago
https://hg.mozilla.org/mozilla-central/annotate/0b69d304f861d0038fb78f1d52b0f5d13ef7c6fe/dom/media/platforms/wrappers/FuzzingWrapper.cpp#l239

The task queue of DecoderCallbackFuzzingWrapper is different from that of the reader. |mCallback->ReleaseMediaResources| should be called on the reader's task queue instead of that of DecoderCallbackFuzzingWrapper.
(Assignee)

Comment 1

2 years ago
Hi jya,
It seems wrong to call |mCallback->ReleaseMediaResources| on the task queue of DecoderCallbackFuzzingWrapper.
Assignee: nobody → jwwang
Blocks: 1213764
Depends on: 1214073
Flags: needinfo?(jyavenard)
Gerald would know this more than I
Flags: needinfo?(jyavenard) → needinfo?(gsquelart)
This wrapper uses an internal queue to avoid having to use a lock, and to make sure all calls into the wrapper are forwarded in the same order.
This is mostly important for the Output/Error/InputExhausted/DrainComplete methods (as they are inter-dependent), but I just applied this usage to ReleaseMediaResources as well for consistency.

ReleaseMediaResources is part of the MediaDataDecoderCallback interface.
The documentation there reads: "Implementation is threadsafe, and can be called on any thread."
https://hg.mozilla.org/mozilla-central/annotate/0b69d304f861d0038fb78f1d52b0f5d13ef7c6fe/dom/media/platforms/PlatformDecoderModule.h#l112

So I assumed it was necessary for me to use my own queue when implementing this interface (to make it thread-safe), and also that it was safe when calling the original callback from my own queue.

Is that documentation incorrect?
Flags: needinfo?(gsquelart)
(Assignee)

Comment 4

2 years ago
I guess it should be rephrased to "Implementation must be threadsafe".

https://hg.mozilla.org/mozilla-central/file/0b69d304f861/dom/media/MediaFormatReader.h#l174
DecoderCallback::ReleaseMediaResources() calls mReader->ReleaseMediaResources() which must happen on the reader's task queue.

According to the document, it is the responsibility of DecoderCallback to ensure the implementation is thread-safe.
(Assignee)

Comment 5

2 years ago
(In reply to Gerald Squelart [:gerald] from comment #3)
> This wrapper uses an internal queue to avoid having to use a lock, and to
> make sure all calls into the wrapper are forwarded in the same order.
> This is mostly important for the Output/Error/InputExhausted/DrainComplete
> methods (as they are inter-dependent), but I just applied this usage to
> ReleaseMediaResources as well for consistency.
I see. Then we should fix DecoderCallback per comment 4.
(In reply to JW Wang [:jwwang] from comment #5)
> I see. Then we should fix DecoderCallback per comment 4.

Or fix MediaFormatReader::ReleaseMediaResources() to dispatch itself to the reader queue, the same way MediaFormatReader::Output and other do; this would be more consistent overall.

In any case, this bug here is probably "invalid" and a new one should be created; or you could just change the title.
(Assignee)

Comment 7

2 years ago
Right. I open bug 1214073 to fix MediaDecoderReader::ReleaseMediaResources. I will close this bug as invalid. Thanks!
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.