1144523 - Convert input buffer to expected format for OpenH264 GMP plugin

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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

[Matthew Gregan [:kinetik]]

Attachment: bug1144523_v0.patch

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8579126 [details] [diff] [review]
bug1144523_v0.patch

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.

Comment 3

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43f2f38859d0

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/43f2f38859d0

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8579126 [details] [diff] [review]
bug1144523_v0.patch

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14e163e39e34