1033902 - Integrate AudioOffloadPlayer with MediaCodecReader and MediaCodecDecoder.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bruce Sun [:brsun]]

This is a followup bug of bug 904177. AudioOffloadPlayer is needed to be integrated with MediaCodecReader and MediaCodecDecoder.

Comment 1

[vasanth]

Would like to know when this will be fixed? 
This has major impact on our audio playback power numbers.

Comment 2

[Bruce Sun [:brsun]]

MediaCodecReader will not be used (bug 1033935) by default if this issue has not been fixed.

Comment 3

[Ivan Tsay (:ITsay)]

This is part of 2.1 feature work.

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1033902_part_1_media_omx_common_decoder_extraction.v1.patch

Part 1: Extract the logic of audio offload playback from MediaOmxDecoder and MediaOmxReader into MediaOmxCommonDecoder and MediaOmxCommonReader.

Comment 5

[Bruce Sun [:brsun]]

Attachment: bug1033902_part_2_media_codec_offload_player_integration.v1.patch

Part 2: Integrate the logic of audio offload playback into MediaCodecDecoder and MediaCodecReader.

Comment 6

[Bruce Sun [:brsun]]

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.

Comment 7

[vasanth]

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?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Bruce Sun [:brsun]]

Attachment: bug1033902_part_1_media_omx_common_decoder_extraction.v2_hg_copy.patch

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 11

[Bruce Sun [:brsun]]

(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?

Comment 12

[Bruce Sun [:brsun]]

Attachment: bug1033902_part_1_media_omx_common_decoder_extraction.checkin.patch

Comment 13

[Bruce Sun [:brsun]]

Attachment: bug1033902_part_2_media_codec_offload_player_integration.checkin.patch

Comment 14

[Bruce Sun [:brsun]]

https://tbpl.mozilla.org/?tree=Try&rev=d021c993c58e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7945750b315a
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f524c020d8

Comment 16

[vasanth]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7945750b315a
https://hg.mozilla.org/mozilla-central/rev/47f524c020d8