Closed Bug 1033902 Opened 6 years ago Closed 6 years ago

Integrate AudioOffloadPlayer with MediaCodecReader and MediaCodecDecoder.

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
feature-b2g 2.1

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(2 files, 3 obsolete files)

This is a followup bug of bug 904177. AudioOffloadPlayer is needed to be integrated with MediaCodecReader and MediaCodecDecoder.
Blocks: 1033935
Blocks: 1033969
Would like to know when this will be fixed? 
This has major impact on our audio playback power numbers.
blocking-b2g: --- → 2.1?
MediaCodecReader will not be used (bug 1033935) by default if this issue has not been fixed.
This is part of 2.1 feature work.
blocking-b2g: 2.1? → ---
feature-b2g: --- → 2.1
Assignee: nobody → brsun
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)
Part 2: Integrate the logic of audio offload playback into MediaCodecDecoder and MediaCodecReader.
Attachment #8468990 - Flags: feedback?(vasanth)
Attachment #8468990 - Flags: feedback?(bhargavg1)
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.
Attachment #8468989 - Flags: review?(roc)
Attachment #8468990 - Flags: review?(roc)
See Also: → 976172
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-
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+
(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?
Attachment #8468990 - Attachment is obsolete: true
Attachment #8468990 - Flags: feedback?(bhargavg1)
Attachment #8473411 - Flags: review+
(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!
https://hg.mozilla.org/mozilla-central/rev/7945750b315a
https://hg.mozilla.org/mozilla-central/rev/47f524c020d8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
See Also: → 1064376
Depends on: 1064376
See Also: 1064376
You need to log in before you can comment on or make changes to this bug.