Support openmax il client mp3 audio playback.

RESOLVED FIXED in Firefox 47

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: alfredo, Assigned: alfredo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [2016-GBT-Y])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
Priority: -- → P2
Whiteboard: [2016-GBT-Y]
Depends on: 1239607
(Assignee)

Comment 1

2 years ago
Created attachment 8710323 [details] [diff] [review]
support_mp3
Assignee: nobody → ayang
Attachment #8710323 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Updated

2 years ago
Attachment #8710323 - Flags: review?(sotaro.ikeda.g)
Created attachment 8713001 [details] [diff] [review]
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.
Attachment #8710323 - Attachment is obsolete: true
Attachment #8713001 - Flags: feedback?(ayang)
(Assignee)

Comment 3

2 years ago
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().
Created attachment 8720686 [details]
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/
Attachment #8720686 - Flags: review?(ayang)
(Assignee)

Updated

2 years ago
Attachment #8720686 - Flags: review?(ayang) → review+
(Assignee)

Comment 6

2 years ago
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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a5f5c32373c
https://hg.mozilla.org/mozilla-central/rev/e6b2fb0874ab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.