Closed Bug 1174721 Opened 6 years ago Closed 5 years ago

[FFOS] Use AudioCompactor for mp3 in Gonk PDM

Categories

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

37 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: bwu, Assigned: bechen)

Details

Attachments

(1 file)

We should use AudioCompactor in Gonk PDM to save memory as bug 960873. 
MediaCodecReader and MediaOmxReader use it as well.
Component: Audio/Video → Audio/Video: Playback
Assignee: nobody → bechen
Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager.
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
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?
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager.
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro
Attachment #8665886 - Attachment description: MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. → MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
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.
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 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().
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
(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 on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
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 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 :)
Attachment #8665886 - Flags: review?(sotaro.ikeda.g) → review+
Hello, this doesn't apply anymore. Could you please refresh the patch? Please add checkin-needed back again when the patch is refreshed.
Flags: needinfo?(bechen)
Keywords: checkin-needed
Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro
Attachment #8665886 - Flags: review+ → review?(sotaro.ikeda.g)
Comment on attachment 8665886 [details]
MozReview Request: Bug 1174721: Use AudioCompactor for GonkAudioDecoderManager. r=sotaro

https://reviewboard.mozilla.org/r/20371/#review20245
Attachment #8665886 - Flags: review+
Flags: needinfo?(bechen)
Attachment #8665886 - Flags: review?(sotaro.ikeda.g)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bd7a7ac80d3d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.