Closed Bug 1020760 Opened 6 years ago Closed 5 years ago

[EME] Extend GMP to support EME plugins

Categories

(Core :: Audio/Video, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 13 obsolete files)

148.09 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
38.82 KB, patch
jesup
: review+
Details | Diff | Splinter Review
13.10 KB, patch
Details | Diff | Splinter Review
We need to change the current GMP API to support AAC decoding of encrypted samples, as required for EME plugins.
Attachment #8434682 - Flags: feedback?(rjesup)
Attachment #8434682 - Flags: feedback?(joshmoz)
We also need to add a function to report errors to the GMPDecoderCallbacks, as the decode could be running async inside the CDM and we need to be able report errors if an async decode operation fails.
Blocks: 1022012
We also need the duration on encoded and decoded video frames.
Comment on attachment 8434682 [details] [diff] [review]
Patch: Proposed GMP changes for EME plugins.

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

Generally this looks good to me, only thing that really concerns me is the main thread stuff.

::: content/media/gmp/GMPTypes.ipdlh
@@ +5,5 @@
>  
>  namespace mozilla {
>  namespace gmp {
>  
> +struct GMPSubsampleEntryData {

We might want to add "EME" to the prefix (e.g. "GMPEMESubsampleEntryData") for this and GMPDecryptionData.

::: content/media/gmp/gmp-api/gmp-audio-decode.h
@@ +38,5 @@
> +#include "gmp-audio-samples.h"
> +#include "gmp-audio-codec.h"
> +#include <stdint.h>
> +
> +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD

Why the main thread and not the GMP thread?

@@ +53,5 @@
> +
> +  virtual void ResetComplete() = 0;
> +};
> +
> +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD

Why the main thread and not the GMP thread?

::: content/media/gmp/gmp-api/gmp-audio-samples.h
@@ +50,5 @@
> +  // The format of the buffer.
> +  virtual GMPAudioFormat GetFormat() = 0;
> +  virtual void Destroy() = 0;
> +
> +  // MAIN THREAD ONLY

Main thread?

::: content/media/gmp/gmp-api/gmp-video-codec.h
@@ +82,5 @@
> +  uint8_t        mConstraints;
> +  uint8_t        mLevel;
> +  uint8_t        mPacketizationMode; // 0 or 1
> +  bool           mFrameDroppingOn;
> +  int            mKeyFrameInterval;

Use a fixed lenth integer.

::: content/media/gmp/mozIGeckoMediaPluginService.idl
@@ +33,5 @@
>    // Callable only on GMP thread.
> +  GMPVideoDecoder getGMPVideoDecoder(in AString aCodec,
> +                                     out GMPVideoHost outVideoHost,
> +                                     [optional] in AString origin,
> +                                     [optional] in AString aKeySystem);

I think we went over a strategy for these arguments in another bug.
Attachment #8434682 - Flags: feedback?(joshmoz) → feedback+
Comment on attachment 8434682 [details] [diff] [review]
Patch: Proposed GMP changes for EME plugins.

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

::: content/media/gmp/gmp-api/gmp-audio-decode.h
@@ +38,5 @@
> +#include "gmp-audio-samples.h"
> +#include "gmp-audio-codec.h"
> +#include <stdint.h>
> +
> +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD

I copied this from GMPDecoderCallback, so you tell me. ;)

I think the confusion here is because this function is called on both sides of the process divide; it's called by the GMP to send data to the parent on the child process' main thread, and it's called on the GMP thread in the parent process to deliver said data to the parent.

So we can just add something to that effect to the comment here.
(In reply to Chris Pearce (:cpearce) from comment #5)
> Comment on attachment 8434682 [details] [diff] [review]
> Patch: Proposed GMP changes for EME plugins.
> 
> Review of attachment 8434682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gmp/gmp-api/gmp-audio-decode.h
> @@ +38,5 @@
> > +#include "gmp-audio-samples.h"
> > +#include "gmp-audio-codec.h"
> > +#include <stdint.h>
> > +
> > +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD
> 
> I copied this from GMPDecoderCallback, so you tell me. ;)
> 
> I think the confusion here is because this function is called on both sides
> of the process divide; it's called by the GMP to send data to the parent on
> the child process' main thread, and it's called on the GMP thread in the
> parent process to deliver said data to the parent.
> 
> So we can just add something to that effect to the comment here.

Yeah, I forgot that. Anything called from the plugin must be called from the main thread, almost everything called from the parent must be called from the GMP thread.
We want to send H.264 frames in AVCC format over to the GMP, so we want to either:
1. send the whole AVCC "extra data" in the GMPVideoCodecH264 and remove the existing fields there that are contained in the extra data, or
2. document in gmp-video-codec.h what format the SPS and PPS are in, i.e. are the SPS/PPS in AnnexB format (with start-codes inbetween records) or in AVCC format.

I'd be happier with 1., as it's slightly less work.
Attached patch WIP patch (obsolete) — Splinter Review
Here's my WIP patch, modifying Gecko Media Plugins to work with Gecko, as a PlatformDecoderModule.

This works with the GMP I have written at:
https://github.com/cpearce/gmp-clearkey
Missing from the above patch but still required to be added to gmp-api for EME: timers, clock, storage, asynchronous shutdown.
And also required; some sort of thing to facilitate Output Protection. We're still not 100% sure what we'll need for OP.
Attachment #8450019 - Flags: feedback?(ethanhugg)
Attachment #8450019 - Flags: feedback?(cpearce)
Comment on attachment 8450019 [details] [diff] [review]
WIP API changes for GMP

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

Looks fine to me. This sort of thing is what I need for video decoding purposes.

::: content/media/gmp/GMPVideoi420FrameImpl.h
@@ +54,5 @@
>    virtual GMPVideoErr SetWidth(int32_t aWidth) MOZ_OVERRIDE;
>    virtual GMPVideoErr SetHeight(int32_t aHeight) MOZ_OVERRIDE;
>    virtual int32_t Width() const MOZ_OVERRIDE;
>    virtual int32_t Height() const MOZ_OVERRIDE;
> +  virtual void SetTimestamp(uint64_t aTimestamp) MOZ_OVERRIDE;

We also need 64bit timestamps and duration on GMPVideoEncodedFrame.

And we need to make these changes to the interfaces in gmp-api headers.

And it would be nice if we could consistently capitalize "Timestamp". We predominantly use "TimeStamp" in most of our other Gecko/C++ code (and in GMPVideoEncodedFrame), so I think we should use "TimeStamp". (I used "Timestamp elsewhere in my EME/GMP patches. I was too lazy to check until now.)

::: content/media/gmp/gmp-api/gmp-video-codec.h
@@ +75,4 @@
>  };
>  
> +// H264 specific
> +struct GMPVideoCodecH264AVCC

The only config things I need are in this struct. So if there's anything else outside of this struct that *you* don't need, feel free to remove it.

@@ +82,5 @@
> +  uint8_t        mConstraints;
> +  uint8_t        mLevel;
> +  uint8_t        mLengthSizeMinusOne; // lower 2 bits (== GMPBufferType-1). Top 6 reserved (1's)
> +  // These will not generally be defined for interactive use unless SDP parameter-sets are used
> +  uint8_t        mNumSPS; // lower 5 bits; top 5 reserved (1's)

"top 3 reserved"? Can we 0 them? Or must they be 1 because of a spec?

@@ +133,5 @@
>    kGMPCodecModeInvalid // Should always be last.
>  };
>  
> +enum GMPApiVersion {
> +  kGMPVersion32 = 1, // leveraging that V32 had mCodecType first, and only supported H264

This is the Gecko version right? Why not just use 32 instead of 1 for kGMPVersion32 then?

@@ +225,5 @@
>  };
>  
> +
> +/**********************************************************/
> +/* Legacy API support */

Why do we need this? We only need to keep the API for the plugin we know are shipping in the wild, which at this moment is exactly none...
Attachment #8450019 - Flags: feedback?(cpearce) → feedback+
(In reply to Chris Pearce (:cpearce) from comment #12)

> ::: content/media/gmp/GMPVideoi420FrameImpl.h
> @@ +54,5 @@
> >    virtual GMPVideoErr SetWidth(int32_t aWidth) MOZ_OVERRIDE;
> >    virtual GMPVideoErr SetHeight(int32_t aHeight) MOZ_OVERRIDE;
> >    virtual int32_t Width() const MOZ_OVERRIDE;
> >    virtual int32_t Height() const MOZ_OVERRIDE;
> > +  virtual void SetTimestamp(uint64_t aTimestamp) MOZ_OVERRIDE;
> 
> We also need 64bit timestamps and duration on GMPVideoEncodedFrame.
> 
> And we need to make these changes to the interfaces in gmp-api headers.

Yup (though I think most of them are already, but I haven't cross-checked all the files).

> 
> And it would be nice if we could consistently capitalize "Timestamp". We
> predominantly use "TimeStamp" in most of our other Gecko/C++ code (and in
> GMPVideoEncodedFrame), so I think we should use "TimeStamp". (I used
> "Timestamp elsewhere in my EME/GMP patches. I was too lazy to check until
> now.)

Timestamp is used consistently in the webrtc code for raw times (either RTP Timestamps, or raw millisecond/us timestamps); we use TimeStamp in gecko code for mozilla::TimeStamp objects, which this isn't.  Since it's not a mozilla::TimeStamp, I'd rather keep it as Timestamp.

> ::: content/media/gmp/gmp-api/gmp-video-codec.h
> @@ +75,4 @@
> >  };
> >  
> > +// H264 specific
> > +struct GMPVideoCodecH264AVCC
> 
> The only config things I need are in this struct. So if there's anything
> else outside of this struct that *you* don't need, feel free to remove it.

I already did.  Right now the only addition I have is the H.264 packetization mode - and that may not be strictly necessary since we also have a max-nal-size on each Encode() call.

> 
> @@ +82,5 @@
> > +  uint8_t        mConstraints;
> > +  uint8_t        mLevel;
> > +  uint8_t        mLengthSizeMinusOne; // lower 2 bits (== GMPBufferType-1). Top 6 reserved (1's)
> > +  // These will not generally be defined for interactive use unless SDP parameter-sets are used
> > +  uint8_t        mNumSPS; // lower 5 bits; top 5 reserved (1's)
> 
> "top 3 reserved"? Can we 0 them? Or must they be 1 because of a spec?

"Spec" (there's no true spec for AVCC unless it's part of something like MP4, but the code and discussion I've read says "reserved, must be 1's")

> 
> @@ +133,5 @@
> >    kGMPCodecModeInvalid // Should always be last.
> >  };
> >  
> > +enum GMPApiVersion {
> > +  kGMPVersion32 = 1, // leveraging that V32 had mCodecType first, and only supported H264
> 
> This is the Gecko version right? Why not just use 32 instead of 1 for
> kGMPVersion32 then?

Actually it's the GMP plugin API rev, and I can't used 0 or 1 because in current use it's the codectype field.  I decided to start with 33 for human convenience, but that's fairly arbitrary.

> @@ +225,5 @@
> >  };
> >  
> > +
> > +/**********************************************************/
> > +/* Legacy API support */
> 
> Why do we need this? We only need to keep the API for the plugin we know are
> shipping in the wild, which at this moment is exactly none...

Well, there are versions of the current plugin being used by people experimenting with it (though one has to install it by-hand, and generally build it yourself, and that was one of the questions from Ethan ("Do I need to support a different API for Gecko 32?" - we likely won't be uplifting all the gmp api changes to 32,)
Comment on attachment 8450019 [details] [diff] [review]
WIP API changes for GMP

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

The other thing I think we should do is switch to use GMPErr instead of GMPVideoErr. I need to add several more interfaces, and I don't see the point in declaring a new GMPXXXErr type every time we add a new interface.
Attachment #8450019 - Attachment is obsolete: true
Attachment #8450019 - Flags: feedback?(ethanhugg)
Comment on attachment 8451376 [details] [diff] [review]
API changes for GMP video codec interface

A coordinating patch will be put up for the Cisco plugin code.

This eliminates the use of start codes to delineate NALs and uses the GMPBufferType (length values between NALs/etc).

The CodecSpecificInfo (including the AVCC data) for initing the codecs is a pain to deal with in ipdl (since we're using the same interface on both sides, I have to stay away from mozilla-specific types like TArray inside a structure I pass down.  Finally it was just simpler to use a std::vector like the frametypes and make it a separate argument.

This includes bits of cpearce's EME patches as needed.
Attachment #8451376 - Flags: review?(ethanhugg)
Attachment #8451376 - Flags: review?(cpearce)
Attached patch openh264_top.patch (obsolete) — Splinter Review
This is really for rbcommons, but I keep getting an error uploading it there.  Please port it over.
Attachment #8451378 - Flags: review?(ethanhugg)
Attached patch openh264_gmp_api.patch (obsolete) — Splinter Review
Mirror the changes in gecko for openh264/gmp-api
Attachment #8451379 - Flags: review?(ethanhugg)
Comment on attachment 8451376 [details] [diff] [review]
API changes for GMP video codec interface

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

Looks good, r=cpearce with the std::vector use removed.

Also, IIRC, you need an IPC peer (like Bent) to review all IPC changes.

::: content/media/gmp/GMPVideoDecoderChild.cpp
@@ +119,3 @@
>    // Ignore any return code. It is OK for this to fail without killing the process.
> +  mVideoDecoder->InitDecode(aCodecSettings, codecSpecific, this,
> +                            aCoreCount);

You can pass aCodecSpecific.Elements() and aCodecSpecific.Length() here instead of passing codecSpecific.

::: content/media/gmp/gmp-api/gmp-video-decode.h
@@ +67,5 @@
>  
>    // aCallback: Subclass should retain reference to it until DecodingComplete
>    //            is called. Do not attempt to delete it, host retains ownership.
>    virtual GMPVideoErr InitDecode(const GMPVideoCodec& aCodecSettings,
> +                                 const std::vector<uint8_t>& aCodecSpecific,

I'm not sure if it's safe to use std::vector here. Is std::vector's binary API stable on all platforms?

I'm concerned what will happen if a GMP is compiled with a version of the STL which is incompatible with the version that *we* compile with.

I we should just pass the a const uint8_t* and uint32_t length here. Then there's no possibility of there being ABI problems.

Ditto for the encode case.
Attachment #8451376 - Flags: review?(cpearce) → review+
Depends on: 1035056
per review and discussion with cpearce on IRC, I removed std::vector from the API surface between the Child code and the Plugin.  This included removing the existing std::vector<> for frame types.  In order to break the dependencies on using the plugin's API in the higher layers of Gecko, I split the GMPVideoEncode/Decode classes so we can use size and foo[]* array, while using InfallibleTArrays in the main code.   NOTE: this are effectively interdiffs; once approved I'll fold with the master patch
Comment on attachment 8451445 [details] [diff] [review]
Remove std::vector<> from GMP API

Mostly boilerplate changes
Attachment #8451445 - Flags: review?(ethanhugg)
Attachment #8451445 - Flags: review?(cpearce)
Comment on attachment 8451445 [details] [diff] [review]
Remove std::vector<> from GMP API

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

Sorry, I really don't like splitting the interface parent/plugin interfaces up. This seems like a lot of complication just to avoid passing in a raw pointer and length from our code.

I have a number of small buffers that I need to send over IPC for decryption information and audio, and I don't want to have to follow this same pattern for them, that would make those interfaces much more verbose and complicated.

The two solutions I can see to move forward are:
1. Use raw pointers in the interface, on both sides of the process boundary.
2. Define a custom virtual interface for the codecSpecific, i.e.:

class GMPCodecSpecificData {
public:
  virtual ~GMPCodecSpecificData() {}
  
  const uint8_t* Get() const = 0;
  
  uint32_t Length() const = 0;
};

The InitDecode() function in gmp-api takes this as a parameter.

Then we can have an implementation of this (say called GMPCodecSpecificDataImpl to follow Josh's lead on naming implementations) that we can declare on the stack, and we pass that concrete implementation across via IPDL. It would be easy to write a custom serializer for the concrete class.

Ditto for your GMPVideoFrameType array.

This is basically what I did for passing audio samples from parent to child, and for passing the decryption initialization vectors.

::: content/media/gmp/GMPVideoEncoderChild.cpp
@@ +78,3 @@
>  
>    // Ignore any return code. It is OK for this to fail without killing the process.
> +  mVideoEncoder->InitEncode(aCodecSettings, aCodecSpecific.Length(), codecSpecific, this,

You don't actually need to copy aCodecSpecific's contents into codecSpecific to get a uint8_t*, this could have just been:

 mVideoEncoder->InitEncode(aCodecSettings, aCodecSpecific.Length(), aCodecSpecific.Elements(), this, ...

codecSpecific's memory is released when the scope exits, as is aCodecSpecific's in the parent, so you don't save on memory allocation by copying it again.
Attachment #8451445 - Flags: review?(cpearce) → review-
I have a number of array buffers I need to send across to the child process. Another option, which would save me defining a lot of quite similar inheritance hierarchies for similar array containers, is to define a template array class in the gmp-api for transport:

// Container to hold arbitrary data. Note size is fixed.
template<typename T>
class GMPBuffer {
public:
  virtual const T* Buffer() const = 0;
  virtual uint32_t Size() const = 0;
};

The functions which we want to accept an array of things in the gmp-api can accept a GMPBuffer<T> instead.

Then in some header in content/media/gmp we define specialization/concrete classes for the types we want, like for uint8[] and GMPVideoFrameType[]:

class GMPUint8Buffer : public GMPBuffer<uint8_t> {
public:
  virtual const uint8_t* Buffer() const { return mData.Elements(); }
  virtual uint32_t Size() const { return mData.Length(); }
  nsTArray<uint8_t> mData;
};

class GMPVideoFrameTypeBuffer : public GMPBuffer<GMPVideoFrameType> {
public:
  virtual const GMPVideoFrameType* Buffer() const { return mData.Elements(); }
  virtual uint32_t Size() const { return mData.Length(); }
  nsTArray<GMPVideoFrameType> mData;
};


.. and then serializers in GMPMessageUtils.h:

template <>
struct ParamTraits<mozilla::gmp::GMPUint8Buffer>
{
  typedef mozilla::gmp::GMPUint8Buffer paramType;

  static void Write(Message* aMsg, const paramType& aParam)
  {
    WriteParam(aMsg, aParam.mData);
  }

  static bool Read(const Message* aMsg, void** aItr, paramType* aResult)
  {
    return ReadParam(aMsg, aItr, &(aResult->mData));
  }

  static void Log(const paramType& aParam, std::wstring* aLog)
  {
  }
};

template <>
struct ParamTraits<mozilla::gmp::GMPVideoFrameTypeBuffer>
{
  typedef mozilla::gmp::GMPVideoFrameTypeBuffer paramType;

  static void Write(Message* aMsg, const paramType& aParam)
  {
    WriteParam(aMsg, aParam.mData);
  }

  static bool Read(const Message* aMsg, void** aItr, paramType* aResult)
  {
    return ReadParam(aMsg, aItr, &(aResult->mData));
  }

  static void Log(const paramType& aParam, std::wstring* aLog)
  {
  }
};

Then we can just import this type into IPDL, declare the IPDL interface as passing the concrete type (GMPUint8Buffer rather than GMPBuffer), when we need to send these over IPC we can declare them on the stack, pass the concrete type to IPDL for transport, and in the Recv() on the child side, pass the buffer via pointer to the GMP, so it gets the virtual interface.

I'm happy to stick this into a patch tomorrow if you like.
Comment on attachment 8451378 [details] [diff] [review]
openh264_top.patch


I posted the patch for OpenH264 on RBCommons here:

https://rbcommons.com/s/OpenH264/r/613/
Comment on attachment 8451376 [details] [diff] [review]
API changes for GMP video codec interface

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

I think this will work fine.  I'd like to put it together and try it out, but it's unclear which other patches this one relies on.

::: content/media/gmp/GMPVideoEncoderChild.cpp
@@ +9,5 @@
>  #include "mozilla/unused.h"
>  #include "GMPVideoEncodedFrameImpl.h"
>  #include "GMPVideoi420FrameImpl.h"
>  
> +//using mozilla::ipc::ProcessChild;

I assume we want just remove this line.
Attachment #8451376 - Flags: review?(ethanhugg) → review+
Attached patch openh264_top.patch (obsolete) — Splinter Review
Updated for the current patches I have up (though likely it will change with one last iteration on the interface for the codecspecific stuff)
Attachment #8451378 - Attachment is obsolete: true
Attachment #8451378 - Flags: review?(ethanhugg)
Attachment #8451744 - Flags: review?(ethanhugg)
Attached patch openh264_gmp_api.patch (obsolete) — Splinter Review
Update the gmp-api directory for openh264
Attachment #8451379 - Attachment is obsolete: true
Attachment #8451379 - Flags: review?(ethanhugg)
Attachment #8451746 - Flags: review?(ethanhugg)
Here's the EME APIs implemented, and a GMPArray<T> type that's used to serialize variable length data.

The AVCC's SPS/PPS is still sent in a 0-length array, I didn't get as far as changing that.
Attachment #8434682 - Attachment is obsolete: true
Attachment #8449131 - Attachment is obsolete: true
Attachment #8434682 - Flags: feedback?(rjesup)
Attachment #8452213 - Flags: feedback?(rjesup)
I will write a patch that only adds the gmp-api's needed for EME tomorow. Right now, my brain is full.
Duplicate of this bug: 1022012
Duplicate of this bug: 1024304
Assignee: nobody → rjesup
Attached patch WIP: gmp-arrays changes (obsolete) — Splinter Review
Interdiff
Attachment #8452646 - Flags: feedback?(rjesup)
Comment on attachment 8452646 [details] [diff] [review]
WIP: gmp-arrays changes

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

::: content/media/gmp/GMPVideoEncoderParent.cpp
@@ +63,5 @@
>    mCallback = aCallback;
>  
> +  // Note: we assume the parent-side caller always passes a
> +  // GMPUint8ArrayImpl. We cannot make that assumption when we
> +  // receive callse from the plugin!

typo

@@ +102,5 @@
>    }
>  
> +  // Note: we assume the parent-side caller always passes a
> +  // GMPVideoFrameTypeArrayImpl. We cannot make that assumption when we
> +  // receive callse from the plugin!

typo

::: content/media/gmp/gmp-api/gmp-arrays.h
@@ +12,5 @@
> + *  notice, this list of conditions and the following disclaimer in
> + *  the documentation and/or other materials provided with the
> + *  distribution.
> + *
> + ** Neither the name of Google nor the names of its contributors may

Check the boilerplates.... a) why not MPL2?  Even if it needs to be BSD-ish, I think "google" is wrong.  Do we need to ping Gerv?

::: content/media/gmp/gmp-api/gmp-video-frame-encoded.h
@@ +35,5 @@
>  #define GMP_VIDEO_FRAME_ENCODED_h_
>  
>  #include <stdint.h>
>  #include "gmp-decryption.h"
> +#include "gmp-video-frame.h"

Needed?  (maybe)
Attachment #8452646 - Flags: feedback?(rjesup) → feedback+
(In reply to Randell Jesup [:jesup] from comment #33)
> Comment on attachment 8452646 [details] [diff] [review]
> WIP: gmp-arrays changes
> 
> Review of attachment 8452646 [details] [diff] [review]:
> -----------------------------------------------------------------
> Check the boilerplates....

Oh yeah, copy paste fail.

> a) why not MPL2?

Because CDMs will be closed source, so the gmp-api headers they use should be Apache 2.

>  Even if it needs to be BSD-ish,
> I think "google" is wrong.  Do we need to ping Gerv?

Copy paste fail I'd assume. I'll check these. We don't need approval to use Apache 2 license in Mozilla code.
Attached patch Patch: update GMP APIs for EME (obsolete) — Splinter Review
This adds the API headers needed to support EME GMPs. I will likely need to change then slightly in the coming weeks, but the APIs that OpenH264 need won't likely change.

I commented out the definition of mSPS[] in GMPVideoCodecH264AVCC, because with unified builds and warning-as-errors, it can sometimes cause build failures on Windows. I tried to make it clear in the comments of GMPVideoCodecH264AVCC that the SPS and PPS follow the struct.

I dropped GMPArray<>, and instead defined interfaces in the parent to proxy calls over to the GMPVideo*Parent::SendXXX() functions. These use the same arguments as the IPDL SendXX functions, so we'll avoid an extra copy when using these, and it will scale cleanly for the plethora of other GMPPXXParent interfaces that I'll need to add.

I also had to add a Destroy() function to GMPTask, as if the GMPTask is allocated by the plugin, it can't be deleted by the Gecko child process, as the plugin will probably be using a different version of operator delete. This was causing GMPRunOnMainThread (the non sync version) to crash.

I also merged GMPVideoErr into GMPErr. This means when I add other interfaces, we won't end up with GMPStorageErr, GMPDecryptorErr, etc, etc, etc...

This patch is based on jesup's patch for Bug 1020090.
Attachment #8452213 - Attachment is obsolete: true
Attachment #8452646 - Attachment is obsolete: true
Attachment #8452213 - Flags: feedback?(rjesup)
Attachment #8452866 - Flags: review?(rjesup)
Comment on attachment 8452866 [details] [diff] [review]
Patch: update GMP APIs for EME

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

It would be better (easier to review) in the future to make things like the error-code switch a separate patch.

::: content/media/gmp/GMPService.h
@@ +13,5 @@
>  #include "mozilla/Mutex.h"
>  #include "nsString.h"
>  #include "nsCOMPtr.h"
>  #include "nsIThread.h"
> +#include "nsString.h"

Ummmm, perhaps this file does't need these two changes?  Or at least one of them should go

::: content/media/gmp/GMPVideoDecoderChild.cpp
@@ +110,5 @@
>    if (!mVideoDecoder) {
>      return false;
>    }
>  
> +  mVideoDecoder->InitDecode(aCodecSettings,

leave the comment?

::: content/media/gmp/GMPVideoEncodedFrameImpl.cpp
@@ +91,5 @@
>    aFrameData.mFrameType() = mFrameType;
>    aFrameData.mSize() = mSize;
>    aFrameData.mCompleteFrame() = mCompleteFrame;
>    aFrameData.mBuffer() = mBuffer;
> +  

ws

::: content/media/gmp/GMPVideoEncoderChild.h
@@ -61,5 @@
>                                const int32_t& aNumberOfCores,
>                                const uint32_t& aMaxPayloadSize) MOZ_OVERRIDE;
>    virtual bool RecvEncode(const GMPVideoi420FrameData& aInputFrame,
>                            const GMPCodecSpecificInfo& aCodecSpecificInfo,
> -                          const InfallibleTArray<int>& aFrameTypes) MOZ_OVERRIDE;

Are these supposed to be InfallibleTArrays, or nsTArrays?

::: content/media/gmp/gmp-api/gmp-audio-samples.h
@@ +57,5 @@
> +  virtual GMPErr SetBufferSize(uint32_t aSize) = 0;
> +
> +  // Size of the buffer in bytes.
> +  virtual uint32_t Size() = 0;
> +  

ws

::: content/media/gmp/gmp-api/gmp-decryption.h
@@ +1,3 @@
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Correct license?

@@ +24,5 @@
> +  // Number of enties returned by ClearBytes and CipherBytes().
> +  virtual uint32_t NumSubsamples() const = 0;
> +
> +  virtual const uint32_t* ClearBytes() const = 0;
> +  

space

@@ +99,5 @@
> +                              GMPDOMException aException,
> +                              uint32_t aSystemCode,
> +                              const char* aMessage,
> +                              uint32_t aMessageLength) = 0;
> +                              

ws

@@ +111,5 @@
> +  virtual void OnKeyIdNotUsable(const char* aSessionId,
> +                                uint32_t aSessionIdLength,
> +                                const uint8_t* aKeyId,
> +                                uint32_t aKeyIdLength) = 0;
> +                             

ws

@@ +147,5 @@
> +// Host API: GMPDecryptorHost
> +class GMPDecryptor {
> +public:
> +
> +  // Sets the 

finish comment

@@ +162,5 @@
> +                             const uint8_t* aInitData,
> +                             uint32_t aInitDataSize,
> +                             GMPSessionType aSessionType) = 0;
> +
> +  virtual void LoadSession(uint32_t aPromiseId,

all the others have comments...

::: content/media/gmp/gmp-api/gmp-storage.h
@@ +1,3 @@
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Is this the right license?  (Maybe, just want to check)

::: content/media/gmp/gmp-api/gmp-video-codec.h
@@ +84,5 @@
> +  uint8_t        mProfile; // these 3 are profile_level_id
> +  uint8_t        mConstraints;
> +  uint8_t        mLevel;
> +  uint8_t        mLengthSizeMinusOne; // lower 2 bits (== GMPBufferType-1). Top 6 reserved (1's)
> +  

trailing space
Attachment #8452866 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #36)
> Comment on attachment 8452866 [details] [diff] [review]
> Patch: update GMP APIs for EME
> 
> Review of attachment 8452866 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: content/media/gmp/GMPVideoEncoderChild.h
> @@ -61,5 @@
> >                                const int32_t& aNumberOfCores,
> >                                const uint32_t& aMaxPayloadSize) MOZ_OVERRIDE;
> >    virtual bool RecvEncode(const GMPVideoi420FrameData& aInputFrame,
> >                            const GMPCodecSpecificInfo& aCodecSpecificInfo,
> > -                          const InfallibleTArray<int>& aFrameTypes) MOZ_OVERRIDE;
> 
> Are these supposed to be InfallibleTArrays, or nsTArrays?

"InfallibleTArray and AutoInfallibleTArray are aliases for nsTArray and
nsAutoTArray."

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#53

And "#define InfallibleTArray nsTArray"

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArrayForwardDeclare.h#36

So they're equivalent, AFAICT. I can make it InfallibleTArray if you prefer to retain the implication of how it's allocated.
Patch updated for review comments.
Assignee: rjesup → cpearce
Attachment #8451376 - Attachment is obsolete: true
Attachment #8451445 - Attachment is obsolete: true
Attachment #8452866 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8451445 - Flags: review?(ethanhugg)
Attachment #8453406 - Flags: review+
Attached patch OpenH264 update patch (obsolete) — Splinter Review
This updates Jesup's patch to update the OpenH264 GMP to work with the new GMP APIs.
Attachment #8453408 - Flags: review?(ethanhugg)
Attachment #8451744 - Attachment is obsolete: true
Attachment #8451744 - Flags: review?(ethanhugg)
Comment on attachment 8451746 [details] [diff] [review]
openh264_gmp_api.patch

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

This is outdated, but I can get these from the other patch.
Attachment #8451746 - Flags: review?(ethanhugg)
Comment on attachment 8453408 [details] [diff] [review]
OpenH264 update patch

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

This will work.   When the github/mozilla/gmp-api is updated I will propose this on our RBCommons for merging into OpenH264.
Attachment #8453408 - Flags: review?(ethanhugg) → review+
Attachment #8451746 - Attachment is obsolete: true
This is based ontop of my latest patch.

I will still need to make a few small changes to the GMPDecryptor after this, but they won't impact OpenH264.
Attachment #8453563 - Flags: review?(rjesup)
Attachment #8453563 - Flags: review?(rjesup) → review+
Updated to work with what just landed.
Set MOZ_GMP_PATH to use with inbound
Attachment #8453408 - Attachment is obsolete: true
Comment on attachment 8453406 [details] [diff] [review]
Patch: GMP APIs update

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

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +492,5 @@
>    }
> +
> +  // XXX At this point, we only will get mode1 data (a single length and a buffer)
> +  // Session_info.cc/etc code needs to change to support mode 0.
> +  MOZ_ASSERT(ntohl(*(reinterpret_cast<uint32_t*>(const_cast<uint8_t*>(aInputImage._buffer)))) ==

This didn't compile on Linux Debug on TBPL, so I removed this line as a bustage fix.:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b20e3db93c

Error:

In file included from /builds/slave/m-in-l64-d-0000000000000000000/build/obj-firefox/media/webrtc/signaling/signaling_ecc/Unified_cpp_signaling_ecc0.cpp:106:0:
/builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp: In member function 'virtual int32_t mozilla::WebrtcGmpVideoDecoder::Decode_g(const webrtc::EncodedImage&, bool, const webrtc::RTPFragmentationHeader*, const webrtc::CodecSpecificInfo*, int64_t)':
/builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:498:77: error: statement-expressions are not allowed outside functions nor in template-argument lists
/builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:498:77: error: statement-expressions are not allowed outside functions nor in template-argument lists
/builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:498:428: error: no matching function for call to 'ValidateAssertConditionType()'
/builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:498:428: note: candidate is:
In file included from /builds/slave/m-in-l64-d-0000000000000000000/build/ipc/chromium/src/base/basictypes.h:14:0,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/ipc/chromium/src/base/logging.h:11,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/ipc/chromium/src/base/linked_ptr.h:40,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/./include/SharedPtr.h:8,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/./include/CC_Common.h:7,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/./include/CC_CallTypes.h:7,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/callcontrol/CC_CallTypes.cpp:5,
                 from /builds/slave/m-in-l64-d-0000000000000000000/build/obj-firefox/media/webrtc/signaling/signaling_ecc/Unified_cpp_signaling_ecc0.cpp:2:
../../../../dist/include/mozilla/Assertions.h:334:6: note: template<class T> void mozilla::detail::ValidateAssertConditionType()
../../../../dist/include/mozilla/Assertions.h:334:6: note:   template argument deduction/substitution failed:
In file included from /builds/slave/m-in-l64-d-0000000000000000000/build/obj-firefox/media/webrtc/signaling/signaling_ecc/Unified_cpp_signaling_ecc0.cpp:106:0:
/builds/slave/m-in-l64-d-0000000000000000000/build/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp:498:428: error: template argument 1 is invalid
make[5]: *** [Unified_cpp_signaling_ecc0.o] Error 1
make[5]: Leaving directory `/builds/slave/m-in-l64-d-0000000000000000000/build/obj-firefox/media/webrtc/signaling/signaling_ecc'
make[4]: *** [media/webrtc/signaling/signaling_ecc/compile] Error 2
No longer depends on: 1035056
https://hg.mozilla.org/mozilla-central/rev/54bd83c54daf
https://hg.mozilla.org/mozilla-central/rev/a95fee89e4be
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.