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

RESOLVED WONTFIX

Status

()

P2
normal
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → jwwang
Blocks: 1213764
(Assignee)

Comment 2

3 years ago
Created attachment 8675627 [details]
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.
Attachment #8675627 - Flags: review?(gsquelart)
(Assignee)

Comment 3

3 years ago
Created attachment 8675628 [details]
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.
Attachment #8675628 - Flags: review?(gsquelart)
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).
Attachment #8675627 - Flags: review?(gsquelart) → review+
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
Attachment #8675628 - Flags: review?(gsquelart) → review+
Priority: -- → P2
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.
Flags: needinfo?(jwwang)
(Assignee)

Comment 7

3 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(jwwang)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.