Closed
Bug 1033902
Opened 10 years ago
Closed 10 years ago
Integrate AudioOffloadPlayer with MediaCodecReader and MediaCodecDecoder.
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: brsun, Assigned: brsun)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
brsun
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brsun
:
review+
|
Details | Diff | Splinter Review |
This is a followup bug of bug 904177. AudioOffloadPlayer is needed to be integrated with MediaCodecReader and MediaCodecDecoder.
Would like to know when this will be fixed? This has major impact on our audio playback power numbers.
blocking-b2g: --- → 2.1?
Assignee | ||
Comment 2•10 years ago
|
||
MediaCodecReader will not be used (bug 1033935) by default if this issue has not been fixed.
Comment 3•10 years ago
|
||
This is part of 2.1 feature work.
blocking-b2g: 2.1? → ---
feature-b2g: --- → 2.1
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → brsun
Assignee | ||
Comment 4•10 years ago
|
||
Part 1: Extract the logic of audio offload playback from MediaOmxDecoder and MediaOmxReader into MediaOmxCommonDecoder and MediaOmxCommonReader.
Attachment #8468989 -
Flags: feedback?(vasanth)
Attachment #8468989 -
Flags: feedback?(bhargavg1)
Assignee | ||
Comment 5•10 years ago
|
||
Part 2: Integrate the logic of audio offload playback into MediaCodecDecoder and MediaCodecReader.
Attachment #8468990 -
Flags: feedback?(vasanth)
Attachment #8468990 -
Flags: feedback?(bhargavg1)
Assignee | ||
Comment 6•10 years ago
|
||
I separate codes into two parts. - Part 1 is to simply extract the common logic of audio offload playback from OmxCodec related classes into parent classes. By doing so, the original audio offload function should still work in MediaOmxDecoder and MediaOmxReader. - Part 2 is to add corresponding logic of audio offload playback into MediaCodec related classes. By doing so, the audio offload function should work in MediaCodecDecoder and MediaCodecReader as well.
Assignee | ||
Updated•10 years ago
|
Attachment #8468989 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8468990 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Comment on attachment 8468989 [details] [diff] [review] bug1033902_part_1_media_omx_common_decoder_extraction.v1.patch Review of attachment 8468989 [details] [diff] [review]: ----------------------------------------------------------------- I like the idea here. Moving Audio Offloading code from MediaOMXDecoder to MediaOMXCommonDecoder and using it as base for MediaOMXDecoder and MediaCodecDecoder. Few suggestions --------------- 1. MediaOMXCommonDecoder doesn't have anything related to OMX. Name seems misleading. Can we name it AudioOffloadDecoder or something like that? 2. We also need to make sure that, if derived classes MediaOMXDecoder and MediaCodecDecoder has to override some functions in MediaOMXCommonDecoder, it has to be done with care. Else it might break audio offloading. Is it ok to make MediaOMXCommonDecoder methods, MOZ_FINAL for that?
Attachment #8468989 -
Flags: feedback?(vasanth) → feedback+
Attachment #8468990 -
Flags: feedback?(vasanth) → feedback+
Comment on attachment 8468989 [details] [diff] [review] bug1033902_part_1_media_omx_common_decoder_extraction.v1.patch Review of attachment 8468989 [details] [diff] [review]: ----------------------------------------------------------------- Please change this patch so you use "hg copy" so MediaOmxCommonDecoder.cpp is a copy of MediaOmxDecoder.cpp and likewise to make MediaOmxCommonDecoder.h a copy of MediaOmxDecoder.h. That will make the patch much clearer and blame analysis easier.
Attachment #8468989 -
Flags: review?(roc) → review-
Attachment #8468990 -
Flags: review?(roc) → review+
Assignee | ||
Comment 9•10 years ago
|
||
This patch is based on attachment 8468989 [details] [diff] [review], the difference is to use |hg copy| for the following files in this patch: - From MediaOmxDecoder.cpp to MediaOmxCommonDecoder.cpp - From MediaOmxDecoder.h to MediaOmxCommonDecoder.h - From MediaOmxReader.cpp to MediaOmxReader.cpp - From MediaOmxReader.h to MediaOmxReader.h
Attachment #8468989 -
Attachment is obsolete: true
Attachment #8468989 -
Flags: feedback?(bhargavg1)
Attachment #8472223 -
Flags: review?(roc)
Comment on attachment 8472223 [details] [diff] [review] bug1033902_part_1_media_omx_common_decoder_extraction.v2_hg_copy.patch Review of attachment 8472223 [details] [diff] [review]: ----------------------------------------------------------------- Much better...
Attachment #8472223 -
Flags: review?(roc) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to vasanth from comment #7) > Comment on attachment 8468989 [details] [diff] [review] > bug1033902_part_1_media_omx_common_decoder_extraction.v1.patch > > Review of attachment 8468989 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the idea here. > Moving Audio Offloading code from MediaOMXDecoder to MediaOMXCommonDecoder > and using it as base for MediaOMXDecoder and MediaCodecDecoder. > Thanks for your appreciation. > Few suggestions > --------------- > > 1. MediaOMXCommonDecoder doesn't have anything related to OMX. > Name seems misleading. Can we name it AudioOffloadDecoder or something like > that? > The basic idea to have a common parent class is to extract not only audio-offload related logics but also all other common logics of MediaOmxDecoder and MediaCodecDecoder into it. So adding audio-offload wordings on the name of that parent class might limit its design. Both MediaOmxDecoder and MediaCodecDecoder indirectly use underlying OMX IL components from libstagefright, so having an OMX wording on the name of that parent class would be one of suitable choices as well. What do you think? > 2. We also need to make sure that, > if derived classes MediaOMXDecoder and MediaCodecDecoder has to override > some functions in MediaOMXCommonDecoder, > it has to be done with care. Else it might break audio offloading. Fully Agree. But I don't have a good idea to address this concerns now. See comments below. > Is it ok to make MediaOMXCommonDecoder methods, MOZ_FINAL for that? The original design of those public virtual functions are meant to be overridable. If we would like to freeze some logics in parent classes, probably we need to finely separate overridable parts from original functions (ex. make xxx_public_interface() be MOZ_FINAL for freezing some logics, and let it call virtual xxx_private_implementation() for original / overridable parts.) But by doing so, we might double the number of member functions, and that might introduce some potential readability and maintenance efforts. Not sure if it is worth to do it now because it seems there are not many demands to modify / override those base functions. How about just keeping an eye on it first, and reconsidering this topic again when demands increase?
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8472223 -
Attachment is obsolete: true
Attachment #8473410 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8468990 -
Attachment is obsolete: true
Attachment #8468990 -
Flags: feedback?(bhargavg1)
Attachment #8473411 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d021c993c58e
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7945750b315a https://hg.mozilla.org/integration/mozilla-inbound/rev/47f524c020d8
Keywords: checkin-needed
Comment 16•10 years ago
|
||
(In reply to Bruce Sun [:brsun] from comment #11) > Both MediaOmxDecoder and MediaCodecDecoder indirectly use underlying OMX IL > components from libstagefright, so having an OMX wording on the name of that > parent class would be one of suitable choices as well. > > What do you think? > > How about just keeping an eye on it first, and reconsidering this topic again > when demands increase? sgtm. thanks!
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7945750b315a https://hg.mozilla.org/mozilla-central/rev/47f524c020d8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•