1174721 - [FFOS] Use AudioCompactor for mp3 in Gonk PDM

Status: RESOLVED FIXED

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

Description

[Blake Wu [:bwu][:blakewu]]

We should use AudioCompactor in Gonk PDM to save memory as bug 960873. 
MediaCodecReader and MediaOmxReader use it as well.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager.

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

https://reviewboard.mozilla.org/r/20371/#review18411

::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp:119
(Diff revision 1)
> -    return NS_ERROR_UNEXPECTED;
> +    return;

Is there a reason to remove NS_ERROR_UNEXPECTED error handling?

Comment 3

[Benjamin Chen [:bechen]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager.

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Comment 5

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/20371/#review18411

> Is there a reason to remove NS_ERROR_UNEXPECTED error handling?

Last week, I only consider the NS_OK and NS_ERROR_NOT_AVAILABLE cases, forgot the NS_ERROR_UNEXPECTED.
I will change back the |CreateAudioData| to return nsresult.

Comment 6

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/20371/#review18411

Using AudioCompactor needs an AudioQueue inside, so we need to clear the AudioQueue when flushing.
And also the AudioCompactor will divide one audio sample into multiples, so we need to handle the EOS properly.

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

https://reviewboard.mozilla.org/r/20371/#review19785

Sorry for the delay. I am back from PTO. As the following comment, the code seems to have a change to leak MediaBuffer. Dont't we need to address it?

::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp:174
(Diff revision 3)
>    err = mDecoder->Output(&mAudioBuffer, READ_OUTPUT_BUFFER_TIMEOUT_US);

If there is a case that mAudioBuffer　is not null when mDecoder->Output(&mAudioBuffer, ...) is called, it seems to leak MediaBuffer. MediaBuffer needs manual reference count change to release it. It is like in MediaCodecProxy::ReleaseMediaBuffer().

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> As the following comment, the code
> seems to have a change to leak MediaBuffer.

Correction:
   have a chance to leak MediaBuffer.

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Comment 10

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/20371/#review19785

> If there is a case that mAudioBuffer　is not null when mDecoder->Output(&mAudioBuffer, ...) is called, it seems to leak MediaBuffer. MediaBuffer needs manual reference count change to release it. It is like in MediaCodecProxy::ReleaseMediaBuffer().

Although I didn't run into the leak case, I think it is better to prevent it.

And since the GonkAudioDecoderManager::Output has many |return| |break| ...
I'm going make |mAudioBuffer| into a local variable, and create a new class for auto release audiobuffer at the end of GonkAudioDecoderManager::Output.

Comment 11

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

https://reviewboard.mozilla.org/r/20371/#review19971

Thanks for updating the patch! In previous patch, we could not know whether the code cause a memory leak. Current pach looks safer :)

Comment 12

[Benjamin Chen [:bechen]]

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

Comment 13

[Nigel Babu [:nigelb]]

Hello, this doesn't apply anymore. Could you please refresh the patch? Please add checkin-needed back again when the patch is refreshed.

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

https://reviewboard.mozilla.org/r/20371/#review20245

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7a7ac80d3d

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bd7a7ac80d3d