Closed Bug 1239607 Opened 4 years ago Closed 4 years ago

Move OMX codec configuration code to platform layer.

Categories

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

Unspecified
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: jhlin, Assigned: jhlin)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
- 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)
Attachment #8709325 - Attachment is obsolete: true
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)
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)
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)
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)
Address review comments by making it possible to support more codec quirks.
Attachment #8710327 - Attachment is obsolete: true
Attachment #8711557 - Flags: feedback?(ayang)
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+
Rebase and carry r+ from Sotaro.
Attachment #8711557 - Attachment is obsolete: true
Attachment #8712475 - Flags: review+
Rebase and carry r+ from Sotaro.
Attachment #8711559 - Attachment is obsolete: true
Attachment #8712476 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/719ca32a6dce
https://hg.mozilla.org/mozilla-central/rev/b746f8099f06
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.