Closed Bug 1271508 Opened 8 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files, 11 obsolete files)

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.
58 bytes, text/x-review-board-request
jya
: review+
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.
58 bytes, text/x-review-board-request
jya
: review+
Details
MozReview Request: Bug 1271508. Part 3 - extract code to the parent class and remove use of mTaskQueue from sub-classes. r=jya.
58 bytes, text/x-review-board-request
jya
: review+
Details
MozReview Request: Bug 1271508. Part 4 - address review comments. r=jya.
58 bytes, text/x-review-board-request
jya
: review+
Details 
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: nobody → jwwang
Blocks: 1270039
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+
Sorry, I pushed the wrong patches for review. There are indeed 3 patches.
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/
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.
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.
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.
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/
Attachment #8750610 - Attachment is obsolete: true
Attachment #8750580 - Attachment is obsolete: true
Attachment #8750632 - Attachment is obsolete: true
Attachment #8750632 - Flags: review?(jyavenard)
Attachment #8750633 - Attachment is obsolete: true
Attachment #8750633 - Flags: review?(jyavenard)
Attachment #8750634 - Attachment is obsolete: true
Attachment #8750634 - Flags: review?(jyavenard)
Attachment #8750653 - Attachment is obsolete: true
Attachment #8750653 - Flags: review?(jyavenard)
Attachment #8750654 - Attachment is obsolete: true
Attachment #8750654 - Flags: review?(jyavenard)
Attachment #8750655 - Attachment is obsolete: true
Attachment #8750655 - Flags: review?(jyavenard)
Attachment #8750677 - Attachment is obsolete: true
Attachment #8750677 - Flags: review?(jyavenard)
Attachment #8750678 - Attachment is obsolete: true
Attachment #8750678 - Flags: review?(jyavenard)
Attachment #8750679 - Attachment is obsolete: true
Attachment #8750679 - Flags: review?(jyavenard)
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+
Thanks for the review! Will merge P3 and P4 when checking in.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: