Convert input buffer to expected format for OpenH264 GMP plugin

RESOLVED FIXED in Firefox 38



3 years ago
3 years ago


(Reporter: kinetik, Assigned: kinetik)



Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)



(1 attachment)



3 years ago
Per bug 1132797, the OpenH264 GMP plugin expects input buffers specified with GMP_BufferLength32, with the length in native format and including the size of the GMP_BufferLength32 field.

(Whereas the existing EME/CDM stuff expects GMP_BufferLength32 to accept unconverted AVCC-like buffers with big-endian 4-byte NAL lengths that do not include the size of the NAL length.)

Resolving this mismatch tricky because it involves synchronizing version changes with various third parties.  This patch separates the concern of making the GMP video decoder work from resolving ambiguities/disagreements in the interpretation of the GMP API.  I'll resolve those issues in bug 1132797.

Comment 1

3 years ago
Created attachment 8579126 [details] [diff] [review]
Attachment #8579126 - Flags: review?(cpearce)
Comment on attachment 8579126 [details] [diff] [review]

Review of attachment 8579126 [details] [diff] [review]:

::: dom/media/fmp4/gmp/GMPVideoDecoder.cpp
@@ +135,5 @@
> +  // The OpenH264 GMP expects GMP_BufferLength32 to behave as specified in
> +  // the GMP API, whereas all other existing GMPs currently expect
> +  // GMP_BufferLength32 (when combined with kGMPVideoCodecH264) to mean
> +  // "like AVCC but restricted to 4-byte NAL lengths".
> +  if (mNeedBufferLengthCompatHack) {

I think you should call this something more like mRecodeNalLengthsToHostEndian or somesuch, so that it describes the action we're to take, rather than giving it a name that doesn't necessarily describe *what* we'll do.

The whole situation here is a bit of a mess, so I think trying to make it unambiguous here would help.

@@ +139,5 @@
> +  if (mNeedBufferLengthCompatHack) {
> +    const int kNALSize = 4;
> +    uint8_t* buf = frame->Buffer();
> +    while (buf < frame->Buffer() + frame->Size() - kNALSize) {
> +      uint32_t size = buf[0] << 24 | buf[1] << 16 | buf[2] << 8 | buf[3];

You could use mozilla::BigEndian::readUint32() here instead.

@@ +171,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    MOZ_ASSERT(mHost && mGMP);
> +  mNeedBufferLengthCompatHack = mGMP->GetDisplayName().EqualsLiteral("gmpopenh264") &&
> +                                mGMP->GetVersion().EqualsLiteral("1.0");

My installed OpenH264 is version "1.3", and they're about to spool a "1.4" and we don't have a known version where this inconsistency is resolved, so it doesn't seem like it makes a lot of sense to check the version here (yet).

We can probably just remove the GetVersion() check, and its implementation.
Attachment #8579126 - Flags: review?(cpearce) → review+
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Comment 5

3 years ago
Comment on attachment 8579126 [details] [diff] [review]

Requesting uplift to aurora to make future EME-related changes to these files easier to migrate to aurora.  These changes are only called and used in non-EME code (it's restricted to the OpenH264 GMP), so the only purpose of uplifting is to reduce potential merge issues with later patches.

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low risk, this code is only used for the OpenH264 GMP
[String/UUID change made/needed]: none
Attachment #8579126 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
Attachment #8579126 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.