1214074 - Unnecessary task dispatch in DecoderCallbackFuzzingWrapper::ReleaseMediaResources()

Status: RESOLVED INVALID

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Hi jya,
It seems wrong to call |mCallback->ReleaseMediaResources| on the task queue of DecoderCallbackFuzzingWrapper.

Comment 2

[Jean-Yves Avenard [:jya]]

Gerald would know this more than I

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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?

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Right. I open bug 1214073 to fix MediaDecoderReader::ReleaseMediaResources. I will close this bug as invalid. Thanks!