1239607 - Move OMX codec configuration code to platform layer.

Status: RESOLVED FIXED

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

Description

[John Lin [:jhlin][:jolin]]

We'll add more and more codec support to OMX PDM.  Moving add codec specific code (supports given MIME type or not, configure the codec for decoding, ...) could ease the process of introducing new types for codec.

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: Let platform layer decide which codec to support and how to configure it.

- Introduce a new function in platform layer used by module to check if given MIME type is supported.
- Move configuration code to platform layer.
- Break platform layer code into 2 sub layers: one for common interface and another for real platform implementations.

Comment 2

[Alfredo Yang (:alfredo)]

Comment on attachment 8709325 [details] [diff] [review]
Let platform layer decide which codec to support and how to configure it.

Review of attachment 8709325 [details] [diff] [review]:
-----------------------------------------------------------------

Basically, I like the idea that initializes the platform-dependent codec in lower layer and query supporting mimetype instead of hard coding.
One suggestion in the patch:
Some formats configurations are fully complied with OMX spec, they should be moved to platform independent class, like OmxPlatformLayer or OmxDataDecoder. I think it could help to reduce the porting effort if we can keep the platform dependent codes as less as possible.

::: dom/media/platforms/omx/GonkOmxPlatformImpl.cpp
@@ +82,5 @@
> +    );
> +  }
> +  // Not supported.
> +  return OmxAudioConfigFunc();
> +}

This part is complied with OMX spec. Could it move to upper layer? OmxPlatformLayer or OmxDataDecoder if possible.
I'd prefer to keeping GonkOmxPlatformImpl as small as possible. So when we need to support another platform like raspberry pi, we can save effort to implement it.

::: dom/media/platforms/omx/OmxPlatformLayer.h
@@ +39,5 @@
> +  }
> +
> +private:
> +  const Func mFunc;
> +};

Maybe we could create a new base class for format configuration which has default implementations with fully complied OMX configuration and the platform dependent can override/extend it to support the platform dependent codec. It could be a simpler way instead of function pointer and static function IMO.

Comment 3

[John Lin [:jhlin][:jolin]]

Attachment: Part 1 - use MediaCodecList to collect codec info

For collecting codec info we don't really need OMXCodec. MediaCodecList is the actual class that does the hard work.

Comment 4

[John Lin [:jhlin][:jolin]]

Attachment: Part 2 - let platform layer decide which codec to support and how to configure it..

Address Alfredo's comments by moving code around:
- make configuration functions extensible and put basic implementations in  platform layer
- each platform implementation decides which codec to support

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8710332 [details] [diff] [review]
Part 2 - let platform layer decide which codec to support and how to configure it..

Obsolete incorrect patch.

Comment 6

[John Lin [:jhlin][:jolin]]

Attachment: Part 2 - let platform layer decide which codec to support and how to configure it.

Upload correct patch.

Comment 7

[Alfredo Yang (:alfredo)]

Comment on attachment 8710334 [details] [diff] [review]
Part 2 - let platform layer decide which codec to support and how to configure it.

Review of attachment 8710334 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/omx/OmxDataDecoder.cpp
@@ +648,5 @@
> +    OmxPlatformLayer::AudioConfigForMime(audioInfo->mMimeType);
> +  MOZ_ASSERT(config.get());
> +
> +  OMX_ERRORTYPE err = config->Apply(*mOmxLayer, *audioInfo);
> +  CHECK_OMX_ERR(err);

Is it possible to combine ConfigAudioCodec() and ConfigVideoCodec() into one ConfigCodec()?

Because mMimeType is in TrackInfo, so we only need one definition in OmxPlatformLayer.h like:
typedef OmxConfig<TrackInfo> OmxCodecConfig;

OpenMax IL is format independent so OmxDataDecoder should be format neutralized too. I've done part of it in bug 1229363 for output part. Combining ConfigAudio/VideoCodec into one is the last piece. :)

::: dom/media/platforms/omx/OmxPlatformLayer.h
@@ +28,5 @@
> +public:
> +  virtual ~OmxConfig() {}
> +
> +  virtual OMX_ERRORTYPE Apply(OmxPromiseLayer& aOmx, const ParamType& aParam) = 0;
> +};

Could you please add comments to describe the purpose of this class?
Thank you.

Comment 8

[Alfredo Yang (:alfredo)]

Comment on attachment 8710327 [details] [diff] [review]
Part 1 - use MediaCodecList to collect codec info

Review of attachment 8710327 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/omx/GonkOmxPlatformLayer.cpp
@@ +648,5 @@
> +
> +    found = true;
> +    if (aAllocateInputBuffer) {
> +      *aAllocateInputBuffer =
> +        codecs->codecHasQuirk(index, "requires-allocate-on-input-ports");

I'd prefer to keeping mQuirks, because there are other values in the android Quirks enumeration [1].

Although we only use kRequiresAllocateBufferOnInputPorts and kRequiresAllocateBufferOnOutputPorts here, it is possible we will use other value like kOutputBuffersAreUnreadable in future.

[1] http://androidxref.com/4.4.2_r2/xref/frameworks/av/include/media/stagefright/OMXCodec.h#89

Comment 9

[John Lin [:jhlin][:jolin]]

Attachment: Part 1 - use MediaCodecList to collect codec info.

Address review comments by making it possible to support more codec quirks.

Comment 10

[John Lin [:jhlin][:jolin]]

Attachment: Part 2 - let platform layer decide which codec to support and how to configure it.

Address review comments:
- unify Config(Audio|Video)Codec() & (Audio|Video)ConfigForMime()
- use mInfo stored in platform layer from Init() rather than passing it as argument of config function.

Comment 11

[Alfredo Yang (:alfredo)]

Comment on attachment 8711557 [details] [diff] [review]
Part 1 - use MediaCodecList to collect codec info.

Review of attachment 8711557 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 12

[Alfredo Yang (:alfredo)]

Comment on attachment 8711559 [details] [diff] [review]
Part 2 - let platform layer decide which codec to support and how to configure it.

Review of attachment 8711559 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thank you.

Comment 13

[John Lin [:jhlin][:jolin]]

Attachment: Part 1 - use MediaCodecList to collect codec info.

Rebase and carry r+ from Sotaro.

Comment 14

[John Lin [:jhlin][:jolin]]

Attachment: bug1239607-platform-layer-conf.patch

Rebase and carry r+ from Sotaro.

Comment 15

[John Lin [:jhlin][:jolin]]

Try result -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6aed33cd40c&selectedJob=15880143

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/719ca32a6dce
https://hg.mozilla.org/integration/mozilla-inbound/rev/b746f8099f06

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/719ca32a6dce
https://hg.mozilla.org/mozilla-central/rev/b746f8099f06

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/719ca32a6dce
https://hg.mozilla.org/mozilla-central/rev/b746f8099f06