Closed Bug 1229360 Opened 4 years ago Closed 4 years ago

Support openmax il client mp3 audio playback.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

(Whiteboard: [2016-GBT-Y])

Attachments

(2 files, 1 obsolete file)

No description provided.
Priority: -- → P2
Depends on: 1239607
Attached patch support_mp3 (obsolete) — Splinter Review
Assignee: nobody → ayang
Attachment #8710323 - Flags: review?(sotaro.ikeda.g)
Attachment #8710323 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8710323 - Attachment is obsolete: true
Attachment #8713001 - Flags: feedback?(ayang)
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
Attachment #8713001 - Flags: feedback?(ayang)
Depends on: 1243681
(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().
Attachment #8720686 - Flags: review?(ayang) → review+
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 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/
Attachment #8720686 - Attachment description: MozReview Request: Bug 1229360 - Configure audio output port. r?alfredo → MozReview Request: Bug 1229360 - Support MP3 audio in OpenMAX PDM. r?alfredo
https://hg.mozilla.org/mozilla-central/rev/9a5f5c32373c
https://hg.mozilla.org/mozilla-central/rev/e6b2fb0874ab
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.