1216029 - MediaDecoderReader::ResetDecode should figure out which thread to dispatch the job

Status: RESOLVED WONTFIX

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

Description

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

MediaDecoderReader::ResetDecode should figure out which thread to dispatch the job to hide the internal thread model of MediaDecoderReader from its client code. The client won't have to worry about on which thread this function should run.

Comment 1

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

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

Comment 2

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

Attachment: MozReview Request: Bug 1216029. Part 1 - rename MediaDecoderReader::ResetDecode and move its overrides out of public sections. r=gerald.

Bug 1216029. Part 1 - rename MediaDecoderReader::ResetDecode and move its overrides out of public sections. r=gerald.

Comment 3

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

Attachment: MozReview Request: Bug 1216029. Part 2 - fix MediaDecoderReader::ResetDecode and its callers. r=gerald.

Bug 1216029. Part 2 - fix MediaDecoderReader::ResetDecode and its callers. r=gerald.

Comment 4

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

Comment on attachment 8675627 [details]
MozReview Request: Bug 1216029. Part 1 - rename MediaDecoderReader::ResetDecode and move its overrides out of public sections. r=gerald.

https://reviewboard.mozilla.org/r/22401/#review19985

r+ with optional nits.

::: dom/media/MediaFormatReader.h:72
(Diff revision 1)
>    virtual bool ForceZeroStartTime() const override;

While you're around, why not remove the unneeded 'virtual'? (It's the only one left in this file.)

::: dom/media/omx/MediaOmxReader.h:120
(Diff revision 1)
> +  virtual nsresult ResetDecodeInternal()

Missing 'override' (same as the old code, but we might as well fix it in passing).

Comment 5

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

Comment on attachment 8675628 [details]
MozReview Request: Bug 1216029. Part 2 - fix MediaDecoderReader::ResetDecode and its callers. r=gerald.

https://reviewboard.mozilla.org/r/22403/#review19989

Comment 6

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

This seems completely unnecessary, and adding an "Internal" suffix to the method makes implementing this clunky. Please back this out.

The MediaDecoderReader has only one user; the state machine, it is not an interface that's exposed to general users. The MediaDecoderReader interface is supposed to only be called on the decode task queue, that's why there are assertions throughout all the implementations ensuring that. That's why we went and have things like ProxyMediaCall to make this happen.

Ditto for your other similar patches.

Comment 7

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

Per discussion of bug 1215003 comment 18 ~ bug 1215003 comment 22, I will backout related bugs and take the approach 2 to create MediaDecoderReaderProxy for handling thread details.