Remove use of mTaskQueue from FFmpeg{Audio,Video}Decoder

RESOLVED FIXED in Firefox 49

Status

()

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

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 11 obsolete attachments)

58 bytes, text/x-review-board-request
jya
: review+
Details | Review
58 bytes, text/x-review-board-request
jya
: review+
Details | Review
58 bytes, text/x-review-board-request
jya
: review+
Details | Review
58 bytes, text/x-review-board-request
jya
: review+
Details | Review
(Assignee)

Description

2 years ago
By separating code about dispatching jobs from code about doing actual decoding jobs, we are able to move all use of mTaskQueue to the parent class (FFmpegDataDecoder).

This will make it easier for changes to mTaskQueue to get rid of FlushableTaskQueue.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1270039
(Assignee)

Comment 2

2 years ago
Created attachment 8750580 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51491/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51491/
Attachment #8750580 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Blocks: 1271517
Comment on attachment 8750580 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

https://reviewboard.mozilla.org/r/51491/#review48275

::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.h:38
(Diff revision 1)
>    {
>      return "ffmpeg audio decoder";
>    }
>  
>  private:
> -  void Decode(MediaRawData* aSample);
> +  DecodeResult DoDecode(MediaRawData* aSample) override;

All methods over time has been named such as Drain() -> ProcessDrain ; Flush() -> ProcessFlush()

Should continue , so Decode->ProcessDecode.

Please rename accordingly

::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp
(Diff revision 1)
> -}
> -
> -void
>  FFmpegAudioDecoder<LIBAV_VER>::ProcessDrain()
>  {
> -  MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());

why remove this assert when we need to be in the decoder's task queue to flush ?

If not, then you're using FFmpeg in a non thread-safe manner.

::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.h:70
(Diff revision 1)
> +  void Decode(MediaRawData* aSample);
> +  virtual DecodeResult DoDecode(MediaRawData* aSample) = 0;
> +  virtual void ProcessDrain() = 0;
> +
>    static StaticMutex sMonitor;
> +  RefPtr<FlushableTaskQueue> mTaskQueue;

can't that be made const RefPtr?

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h:37
(Diff revision 1)
>    virtual ~FFmpegVideoDecoder();
>  
>    RefPtr<InitPromise> Init() override;
> -  nsresult Input(MediaRawData* aSample) override;
> -  void ProcessDrain() override;
>    void ProcessFlush() override;

Why make ProcessDrain private, but keep ProcessFlush public?

this is not consistent.

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
(Diff revision 1)
> -}
> -
> -void
>  FFmpegVideoDecoder<LIBAV_VER>::ProcessDrain()
>  {
> -  MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());

Same here, I don't think this assert should be removed.
Attachment #8750580 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 4

2 years ago
Sorry, I pushed the wrong patches for review. There are indeed 3 patches.
(Assignee)

Comment 5

2 years ago
Created attachment 8750609 [details]
MozReview Request: Bug 1271508. Part 1 - refactor FFmpegAudioDecoder code to be similar to FFmpegVideoDecoder::Input() so it would be easier to extract common code to the parent class. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51551/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51551/
Attachment #8750609 - Flags: review?(jyavenard)
Attachment #8750610 - Flags: review?(jyavenard)
(Assignee)

Comment 6

2 years ago
Created attachment 8750610 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51553/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51553/
(Assignee)

Comment 7

2 years ago
Comment on attachment 8750580 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51491/diff/1-2/
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/51491/#review48275

> why remove this assert when we need to be in the decoder's task queue to flush ?
> 
> If not, then you're using FFmpeg in a non thread-safe manner.

We want to move all use of mTaskQueue to FFmpegDataDecoder which will ensure all funtions are used in a thread-safe manner. Or do you prefer to add a protected AssertOnTaskQueue() from FFmpegDataDecoder so sub-classes can check the running thread?

> Why make ProcessDrain private, but keep ProcessFlush public?
> 
> this is not consistent.

Will fix that in patch 4.

> Same here, I don't think this assert should be removed.

See the reason above. The goal of this bug is separating code about dispatching jobs (FFmpegDataDecoder) from code about doing acutal decoding (FFmpeg{Audio,Video}Decoder). I can expose AssertOnTaskQueue() from the parent class if it is preferred.
Attachment #8750609 - Flags: review?(jyavenard) → review+
Comment on attachment 8750609 [details]
MozReview Request: Bug 1271508. Part 1 - refactor FFmpegAudioDecoder code to be similar to FFmpegVideoDecoder::Input() so it would be easier to extract common code to the parent class. r=jya.

https://reviewboard.mozilla.org/r/51551/#review48303

::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp:110
(Diff revision 1)
>    packet.data = const_cast<uint8_t*>(aSample->Data());
>    packet.size = aSample->Size();
>  
>    if (!PrepareFrame()) {
>      NS_WARNING("FFmpeg audio decoder failed to allocate frame.");
>      mCallback->Error();

While at it, it would be nicer to handle the callback Error in the DoDecode's caller instead...

This could be done in FFMpegDataDecoder instead, putting in common the code.
Attachment #8750610 - Flags: review?(jyavenard) → review+
Comment on attachment 8750610 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

https://reviewboard.mozilla.org/r/51553/#review48305

nice, this was on my todo list...

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
(Diff revision 1)
>  
>  FFmpegVideoDecoder<LIBAV_VER>::DecodeResult
> -FFmpegVideoDecoder<LIBAV_VER>::DoDecodeFrame(MediaRawData* aSample,
> +FFmpegVideoDecoder<LIBAV_VER>::DoDecode(MediaRawData* aSample,
> -                                            uint8_t* aData, int aSize)
> +                                        uint8_t* aData, int aSize)
>  {
> -  MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());

Removing this assert is too early and out of scope of this patch.
Attachment #8750580 - Flags: review+
Comment on attachment 8750580 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

https://reviewboard.mozilla.org/r/51491/#review48307

::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp:118
(Diff revision 2)
>  
> +void
> +FFmpegDataDecoder<LIBAV_VER>::Decode(MediaRawData* aSample)
> +{
> +  MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());
> +  if (DoDecode(aSample) != DECODE_ERROR && mTaskQueue->IsEmpty()) {

As per comment in P1.

Calling of mCallback->Error() should be removed from DoDecode and handled here.
(Assignee)

Comment 12

2 years ago
https://reviewboard.mozilla.org/r/51491/#review48307

> As per comment in P1.
> 
> Calling of mCallback->Error() should be removed from DoDecode and handled here.

Will address that in P4.
(Assignee)

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/51553/#review48305

> Removing this assert is too early and out of scope of this patch.

Oops! Will move that to P3.
(Assignee)

Comment 14

2 years ago
Created attachment 8750632 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51563/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51563/
Attachment #8750632 - Flags: review?(jyavenard)
Attachment #8750633 - Flags: review?(jyavenard)
Attachment #8750634 - Flags: review?(jyavenard)
(Assignee)

Comment 15

2 years ago
Created attachment 8750633 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51565/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51565/
(Assignee)

Comment 16

2 years ago
Created attachment 8750634 [details]
MozReview Request: Bug 1271508. Part 4 - address review comments. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51567/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51567/
(Assignee)

Comment 17

2 years ago
Comment on attachment 8750609 [details]
MozReview Request: Bug 1271508. Part 1 - refactor FFmpegAudioDecoder code to be similar to FFmpegVideoDecoder::Input() so it would be easier to extract common code to the parent class. r=jya.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51551/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8750610 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8750580 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Created attachment 8750653 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51573/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51573/
Attachment #8750653 - Flags: review?(jyavenard)
Attachment #8750654 - Flags: review?(jyavenard)
Attachment #8750655 - Flags: review?(jyavenard)
(Assignee)

Comment 19

2 years ago
Created attachment 8750654 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51575/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51575/
(Assignee)

Comment 20

2 years ago
Created attachment 8750655 [details]
MozReview Request: Bug 1271508. Part 4 - address review comments. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51577/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51577/
(Assignee)

Updated

2 years ago
Attachment #8750632 - Attachment is obsolete: true
Attachment #8750632 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8750633 - Attachment is obsolete: true
Attachment #8750633 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8750634 - Attachment is obsolete: true
Attachment #8750634 - Flags: review?(jyavenard)
(Assignee)

Comment 21

2 years ago
Created attachment 8750677 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51579/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51579/
Attachment #8750677 - Flags: review?(jyavenard)
Attachment #8750678 - Flags: review?(jyavenard)
Attachment #8750679 - Flags: review?(jyavenard)
(Assignee)

Comment 22

2 years ago
Created attachment 8750678 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51581/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51581/
(Assignee)

Comment 23

2 years ago
Created attachment 8750679 [details]
MozReview Request: Bug 1271508. Part 4 - address review comments. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51583/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51583/
(Assignee)

Updated

2 years ago
Attachment #8750653 - Attachment is obsolete: true
Attachment #8750653 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8750654 - Attachment is obsolete: true
Attachment #8750654 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8750655 - Attachment is obsolete: true
Attachment #8750655 - Flags: review?(jyavenard)
(Assignee)

Comment 24

2 years ago
Created attachment 8750691 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51589/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51589/
Attachment #8750691 - Flags: review?(jyavenard)
Attachment #8750692 - Flags: review?(jyavenard)
Attachment #8750693 - Flags: review?(jyavenard)
(Assignee)

Comment 25

2 years ago
Created attachment 8750692 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51591/
(Assignee)

Comment 26

2 years ago
Created attachment 8750693 [details]
MozReview Request: Bug 1271508. Part 4 - address review comments. r=jya.

Review commit: https://reviewboard.mozilla.org/r/51593/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51593/
(Assignee)

Updated

2 years ago
Attachment #8750677 - Attachment is obsolete: true
Attachment #8750677 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8750678 - Attachment is obsolete: true
Attachment #8750678 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8750679 - Attachment is obsolete: true
Attachment #8750679 - Flags: review?(jyavenard)
(Assignee)

Comment 27

2 years ago
Too bad I can't fix the history...
Comment on attachment 8750691 [details]
MozReview Request: Bug 1271508. Part 2 - rename functions so they are the same as those of FFmpegAudioDecoder so it would be easier to extract common code to the parent class. r=jya.

https://reviewboard.mozilla.org/r/51589/#review48387
Attachment #8750691 - Flags: review?(jyavenard) → review+
Comment on attachment 8750692 [details]
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.

https://reviewboard.mozilla.org/r/51591/#review48389
Attachment #8750692 - Flags: review?(jyavenard) → review+
Comment on attachment 8750693 [details]
MozReview Request: Bug 1271508. Part 4 - address review comments. r=jya.

https://reviewboard.mozilla.org/r/51593/#review48395

can you please merge this with part 3. thanks
Attachment #8750693 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 31

2 years ago
Thanks for the review! Will merge P3 and P4 when checking in.

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83f7f1abedaf
https://hg.mozilla.org/mozilla-central/rev/a94dbda04021
https://hg.mozilla.org/mozilla-central/rev/2250278b7ab7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.