1229360 - Support openmax il client mp3 audio playback.

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: support_mp3

Comment 2

[John Lin [:jhlin][:jolin]]

Attachment: Support MP3 audio

Rewrite patch:
- rebase on bug 1239607.
- unlike previous one, this version follows the behavior of ACodec in stagefright and doesn't set parameters for input port. Looks odd but it works.

Comment 3

[Alfredo Yang (:alfredo)]

Comment on attachment 8713001 [details] [diff] [review]
Support MP3 audio

Review of attachment 8713001 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/omx/OmxPlatformLayer.cpp
@@ +78,5 @@
> +    OMX_ERRORTYPE err;
> +
> +    OMX_PARAM_PORTDEFINITIONTYPE def;
> +    InitOmxParameter(&def);
> +    def.nPortIndex = 1; // output port

Should we configure input port too?

And although the output port number is always 1 in Gonk, but it may not the same in other platforms especially this class is the common layer. It should be better to iterate the port index and find out the output port like [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/media/platforms/omx/OmxDataDecoder.cpp#688

Comment 4

[John Lin [:jhlin][:jolin]]

(In reply to Alfredo Yang (:alfredo) from comment #3)
> Comment on attachment 8713001 [details] [diff] [review]
> Support MP3 audio
> 
> Review of attachment 8713001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/omx/OmxPlatformLayer.cpp
> @@ +78,5 @@
> > +    OMX_ERRORTYPE err;
> > +
> > +    OMX_PARAM_PORTDEFINITIONTYPE def;
> > +    InitOmxParameter(&def);
> > +    def.nPortIndex = 1; // output port
> 
> Should we configure input port too?

 Although it plays MP3 normally but I agree that both input and output ports should be configured. I'll update the patch.

 BTW, AAC configuration doesn't set parameters for output port. I'll fix that too.

> 
> And although the output port number is always 1 in Gonk, but it may not the
> same in other platforms especially this class is the common layer. It should
> be better to iterate the port index and find out the output port like [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/media/platforms/omx/
> OmxDataDecoder.cpp#688

Turns out spec defines "port base" which clients can query and use to calculate input and output port index. Bug 1243681 was filed to deprecate GetOmxPortIndex().

Comment 5

[John Lin [:jhlin][:jolin]]

Attachment: MozReview Request: Bug 1229360 - Support MP3 audio in OpenMAX PDM. r?alfredo

Review commit: https://reviewboard.mozilla.org/r/35423/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35423/

Comment 6

[Alfredo Yang (:alfredo)]

Comment on attachment 8720686 [details]
MozReview Request: Bug 1229360 - Support MP3 audio in OpenMAX PDM. r?alfredo

https://reviewboard.mozilla.org/r/35423/#review32265

LGTM

Comment 7

[John Lin [:jhlin][:jolin]]

Comment on attachment 8720686 [details]
MozReview Request: Bug 1229360 - Support MP3 audio in OpenMAX PDM. r?alfredo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35423/diff/1-2/

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5f5c32373c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b2fb0874ab

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a5f5c32373c
https://hg.mozilla.org/mozilla-central/rev/e6b2fb0874ab