Move OMX codec configuration code to platform layer.

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jhlin, Assigned: jhlin)

Tracking

Trunk
mozilla47
Unspecified
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8709325 [details] [diff] [review]
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.
Attachment #8709325 - Flags: feedback?(ayang)
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.
Attachment #8709325 - Flags: feedback?(ayang)
(Assignee)

Updated

3 years ago
Attachment #8709325 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8710327 [details] [diff] [review]
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.
Attachment #8710327 - Flags: feedback?(ayang)
(Assignee)

Comment 4

3 years ago
Created attachment 8710332 [details] [diff] [review]
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
Attachment #8710332 - Flags: feedback?(ayang)
(Assignee)

Comment 5

3 years ago
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.
Attachment #8710332 - Attachment is obsolete: true
Attachment #8710332 - Flags: feedback?(ayang)
(Assignee)

Comment 6

3 years ago
Created attachment 8710334 [details] [diff] [review]
Part 2 - let platform layer decide which codec to support and how to configure it.

Upload correct patch.
Attachment #8710334 - Flags: feedback?(ayang)
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.
Attachment #8710334 - Flags: feedback?(ayang)
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
Attachment #8710327 - Flags: feedback?(ayang)
(Assignee)

Comment 9

3 years ago
Created attachment 8711557 [details] [diff] [review]
Part 1 - use MediaCodecList to collect codec info.

Address review comments by making it possible to support more codec quirks.
Attachment #8710327 - Attachment is obsolete: true
Attachment #8711557 - Flags: feedback?(ayang)
(Assignee)

Comment 10

3 years ago
Created attachment 8711559 [details] [diff] [review]
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.
Attachment #8710334 - Attachment is obsolete: true
Attachment #8711559 - Flags: feedback?(ayang)
Comment on attachment 8711557 [details] [diff] [review]
Part 1 - use MediaCodecList to collect codec info.

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

LGTM
Attachment #8711557 - Flags: review?(sotaro.ikeda.g)
Attachment #8711557 - Flags: feedback?(ayang)
Attachment #8711557 - Flags: feedback+
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.
Attachment #8711559 - Flags: review?(sotaro.ikeda.g)
Attachment #8711559 - Flags: feedback?(ayang)
Attachment #8711559 - Flags: feedback+
Attachment #8711557 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8711559 - Flags: review?(sotaro.ikeda.g) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8712475 [details] [diff] [review]
Part 1 - use MediaCodecList to collect codec info.

Rebase and carry r+ from Sotaro.
Attachment #8711557 - Attachment is obsolete: true
Attachment #8712475 - Flags: review+
(Assignee)

Comment 14

3 years ago
Created attachment 8712476 [details] [diff] [review]
bug1239607-platform-layer-conf.patch

Rebase and carry r+ from Sotaro.
Attachment #8711559 - Attachment is obsolete: true
Attachment #8712476 - Flags: review+

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/719ca32a6dce
https://hg.mozilla.org/mozilla-central/rev/b746f8099f06
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.