Closed Bug 1315850 Opened 8 years ago Closed 8 years ago

[EME] Implement ChromiumCDM as native GMP API

Categories

(Core :: Audio/Video: GMP, defect, P3)

Product:
Core
Component:
Audio/Video: GMP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(34 files)

Bug 1315850 - Add PChromiumCDM.ipdl for Widevine CDM.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add pref to toggle on new CDM decoder backend.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add media.eme.chromium-api.enabled to ContentPrefs.cpp.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1315850 - Add GMPService::GetContentChild() with unresolved NodeId.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add LaunchGMPForNodeId to allowed sync IPCs.
59 bytes, text/x-review-board-request
Details
Bug 1315850 - Add GMPService::GetCDM.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add GMP_LOG macro.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add ChromiumAdapter which we can use instead of WidevineAdapter.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Implement trivial cdm::Host functions.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add GetGMPAbstractThread() to GMPUtils.h.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Stub out ChromiumCDMProxy.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Create ChromiumCDMProxy in MediaKeys.cpp when preffed on.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Ensure we query for the correct string in HavePluginForKeySystem.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Implement ChromiumCDMProxy initialization.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Handle sending and receiving key session messages.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Connect MediaKeys.createSession to Chromium CDM.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add more logging to Chromium CDM actors.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Send decrypt operations to Chromium CDM.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Ensure GMPParent checks whether the adapter version is present.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Add CDMProxy::AsChromiumCDMProxy().
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Stub out ChromiumCDMVideoDecoder.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Create CDM video decoder in EMEDecoderModule.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Add threadsafe ChromiumCDMProxy::GetCDMParent.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Initialize video decoder.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Implement video decoding through CDM.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Implement CDM video decoder flush.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Implement CDM video decoder drain.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Shutdown CDMVideoDecoder.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1315850 - Shutdown ChromiumCDMParent.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Hook up CDM storage.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Implement CDM persistent sessions.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Rename DetailedPromise Status enum.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Port the work around from Bug 1343140 to the new CDM video decoder architecture.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1315850 - Ask the GMPService for the GMP thread in GMPParent::ChildTerminated.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
We need to implement the Chromium content_decryption_module.h API as a native GMP API, so that we don't have a 2 to 1 mapping of GMPDecryptor+GMPVideoDecoder to cdm::ContentDecryptionModule8. That mapping means we can't have multiple CDMs per CDM process, as we can't associate GMP actors with each other. Once we re-implement using a native Chromium GMP API, we can use shmems for decrypted audio samples and we can eliminate the extra copy required for video frames.
Comment on attachment 8846945 [details]
Bug 1315850 - Add PChromiumCDM.ipdl for Widevine CDM.

https://reviewboard.mozilla.org/r/119950/#review121846

r+ with nits:

::: dom/media/gmp/ChromiumCDMChild.h:16
(Diff revision 1)
> +namespace mozilla {
> +namespace gmp {
> +
> +class GMPContentChild;
> +
> +class ChromiumCDMChild : public PChromiumCDMChild {

'{' on separate line.

::: dom/media/gmp/ChromiumCDMChild.h:25
(Diff revision 1)
> +  explicit ChromiumCDMChild(GMPContentChild* aPlugin);
> +
> +protected:
> +  ~ChromiumCDMChild() {}
> +
> +  mozilla::ipc::IPCResult RecvInit(const bool& aAllowDistinctiveIdentifier,

No need for the 'mozilla::' namespace qualifier here and in the cpp, as we're already inside the mozilla namespace.

::: dom/media/gmp/ChromiumCDMChild.h:32
(Diff revision 1)
> +  mozilla::ipc::IPCResult RecvSetServerCertificate(const uint32_t& aPromiseId,
> +                                                   nsTArray<uint8_t>&& aServerCert) override;
> +  mozilla::ipc::IPCResult RecvCreateSessionAndGenerateRequest(const uint32_t& aPromiseId,
> +                                                              const uint32_t& aSessionType,
> +                                                              const uint32_t& aInitDataType,
> +                                                              nsTArray<uint8_t>&& aInitData) override;

Looks like way more than 80 columns!

::: dom/media/gmp/ChromiumCDMChild.h:48
(Diff revision 1)
> +  mozilla::ipc::IPCResult RecvResetVideoDecoder() override;
> +  mozilla::ipc::IPCResult RecvDecryptAndDecodeFrame(const CDMInputBuffer& aBuffer) override;
> +  mozilla::ipc::IPCResult RecvDestroy() override;
> +
> +  GMPContentChild* mPlugin;
> +

Remove empty line.

::: dom/media/gmp/ChromiumCDMParent.h:9
(Diff revision 1)
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/gmp/PChromiumCDMParent.h"
> +#include "GMPMessageUtils.h"
> +#include "GMPCrashHelper.h"
> +#include "GMPCrashHelperHolder.h"

Please alphabetize #include's in all files, or at least new files (unless it causes build issues, but that would be wrong too!)

::: dom/media/gmp/ChromiumCDMParent.h:28
(Diff revision 1)
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ChromiumCDMParent)
> +
> +  explicit ChromiumCDMParent(GMPContentParent *aContentParent);
> +
> +  // TODO: Add functions to for clients to send data to CDM, and

Remove first 'to'.

::: dom/media/gmp/ChromiumCDMParent.h:31
(Diff revision 1)
> +  explicit ChromiumCDMParent(GMPContentParent *aContentParent);
> +
> +  // TODO: Add functions to for clients to send data to CDM, and
> +  // a Close() function.
> +
> +protected:

A bit strange to see 'protected' in a 'final' class, but I guess it's consistent with the child, so it's fine if you prefer that.

::: dom/media/gmp/PChromiumCDM.ipdl:52
(Diff revision 1)
> +  async DecryptAndDecodeFrame(CDMInputBuffer aBuffer);
> +
> +  async Destroy();
> +
> +parent:
> +  async __delete__();

According to the C++ standards, adjacent underscores are reserved to the compiler&library implementation.
But I see that this __delete__ has already been used all over the place, so it's probably fine to continue with it... Separate bug?
Attachment #8846945 - Flags: review?(gsquelart) → review+
Comment on attachment 8846946 [details]
Bug 1315850 - Add pref to toggle on new CDM decoder backend.

https://reviewboard.mozilla.org/r/119952/#review121856
Attachment #8846946 - Flags: review?(gsquelart) → review+
Comment on attachment 8846965 [details]
Bug 1315850 - Stub out ChromiumCDMVideoDecoder.

https://reviewboard.mozilla.org/r/119990/#review121954

::: dom/media/platforms/agnostic/eme/ChromiumCDMVideoDecoder.h:17
(Diff revision 1)
> +namespace mozilla {
> +
> +class CDMProxy;
> +struct GMPVideoDecoderParams;
> +
> +class ChromiumCDMVideoDecoder : public MediaDataDecoder {

{ on the following line

::: dom/media/platforms/agnostic/eme/ChromiumCDMVideoDecoder.cpp:43
(Diff revision 1)
> +}
> +
> +RefPtr<MediaDataDecoder::FlushPromise>
> +ChromiumCDMVideoDecoder::Flush()
> +{
> +  return MediaDataDecoder::FlushPromise::CreateAndReject(

MediaDataDecoder:: is unecessary as ChromiumCDMVideoDecoder is inheriting from MediaDataDecoder
Attachment #8846965 - Flags: review?(jyavenard) → review+
Comment on attachment 8846966 [details]
Bug 1315850 - Create CDM video decoder in EMEDecoderModule.

https://reviewboard.mozilla.org/r/119992/#review121958

::: dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp:356
(Diff revision 1)
>    if (SupportsMimeType(aParams.mConfig.mMimeType, nullptr)) {
>      // GMP decodes. Assume that means it can decrypt too.
>      RefPtr<MediaDataDecoderProxy> wrapper =
>        CreateDecoderWrapper(mProxy, aParams);
>      auto params = GMPVideoDecoderParams(aParams);
> +    if (MediaPrefs::EMEChromiumAPIEnabled()) {

this assumes the ChromiumCDMVideoDecoder can decode all video codec...

which I know is true, but can't we have a way to abstract that and bubble up that information rather than having those assumptions?

seems like we're limiting ourselves to what kind of CDM we will support in the future.
Attachment #8846966 - Flags: review?(jyavenard) → review+
Comment on attachment 8846968 [details]
Bug 1315850 - Initialize video decoder.

https://reviewboard.mozilla.org/r/119996/#review121960

::: dom/media/gmp/ChromiumCDMChild.cpp:358
(Diff revision 1)
>  }
>  
>  mozilla::ipc::IPCResult
>  ChromiumCDMChild::RecvInitializeVideoDecoder(const CDMVideoDecoderConfig& aConfig)
>  {
> -  GMP_LOG("ChromiumCDMChild::RecvInitializeVideoDecoder()");
> +  MOZ_ASSERT(!mDecoderInitialized);

can we have some assertions indicating on which target thread everything is running ?

::: dom/media/gmp/ChromiumCDMChild.cpp:377
(Diff revision 1)
>  }
>  
>  mozilla::ipc::IPCResult
>  ChromiumCDMChild::RecvDeinitializeVideoDecoder()
>  {
>    GMP_LOG("ChromiumCDMChild::RecvDeinitializeVideoDecoder()");

under which circumstances will RecvDeinitalizeVideoDecoder be received?

is that called before shutting down?

I'm not sure of the use of a mDecoderInitialized member tbh. If init failed, then none of those methods will ever be called. it's a strong given.

Adding even more assertion just muddy things imho.

::: dom/media/gmp/ChromiumCDMChild.cpp:409
(Diff revision 1)
>  mozilla::ipc::IPCResult
>  ChromiumCDMChild::RecvDestroy()
>  {
>    GMP_LOG("ChromiumCDMChild::RecvDestroy()");
>  
> +  MOZ_ASSERT(!mDecoderInitialized);

assuming this is called when Shutdown() is called, that could in theory be happening after initialisation failed or even before the initialisation completed and got interrupted.

so the assert is wrong

::: dom/media/gmp/ChromiumCDMParent.h:16
(Diff revision 1)
>  #include "GMPMessageUtils.h"
>  #include "GMPCrashHelper.h"
>  #include "GMPCrashHelperHolder.h"
>  #include "nsDataHashtable.h"
>  #include "DecryptJob.h"
> +#include "PlatformDecoderModule.h"

include should be alphabetically ordered

::: dom/media/gmp/ChromiumCDMParent.cpp:422
(Diff revision 1)
>  
> +RefPtr<MediaDataDecoder::InitPromise>
> +ChromiumCDMParent::InitializeVideoDecoder(const gmp::CDMVideoDecoderConfig& aConfig)
> +{
> +  if (!SendInitializeVideoDecoder(aConfig)) {
> +    return MediaDataDecoder::InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);

MediaDataDecoder:: is unecessary, and that doesn't look like it fits in 80 columns

::: dom/media/gmp/ChromiumCDMParent.cpp:422
(Diff revision 1)
>  
> +RefPtr<MediaDataDecoder::InitPromise>
> +ChromiumCDMParent::InitializeVideoDecoder(const gmp::CDMVideoDecoderConfig& aConfig)
> +{
> +  if (!SendInitializeVideoDecoder(aConfig)) {
> +    return MediaDataDecoder::InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);

adding a description detailing why that failed would be nice (and wherever a promise is rejected)

::: dom/media/gmp/ChromiumCDMParent.cpp:425
(Diff revision 1)
> +{
> +  if (!SendInitializeVideoDecoder(aConfig)) {
> +    return MediaDataDecoder::InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);
> +  }
> +
> +  RefPtr<MediaDataDecoder::InitPromise> promise = mInitVideoDecoderPromise.Ensure(__func__);

can do return mInitVideoDecoderPromise.Ensure(__func__)

::: dom/media/platforms/agnostic/eme/ChromiumCDMVideoDecoder.cpp:13
(Diff revision 1)
>  #include "GMPService.h"
>  #include "GMPVideoDecoder.h"
> +#include "ChromiumCDMProxy.h"
> +#include "VPXDecoder.h"
> +#include "MP4Decoder.h"
> +#include "content_decryption_module.h"

#include not alphabetically ordered (except for the first ChromiumCDMVideoDecoder.h)

::: dom/media/platforms/agnostic/eme/ChromiumCDMVideoDecoder.cpp:53
(Diff revision 1)
>  {
> -  return MediaDataDecoder::InitPromise::CreateAndResolve(true, __func__);
> +  if (!mCDMParent) {
> +    // Must have failed to get the CDMParent from the ChromiumCDMProxy
> +    // in our constructor; the MediaKeys must have shut down the CDM
> +    // before we had a chance to start up the decoder.
> +    return MediaDataDecoder::InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);

remove MediaDataDecoder::

80 columns formatting too

::: dom/media/platforms/agnostic/eme/ChromiumCDMVideoDecoder.cpp:84
(Diff revision 1)
>  }
>  
>  RefPtr<MediaDataDecoder::DecodePromise>
>  ChromiumCDMVideoDecoder::Decode(MediaRawData * aSample)
>  {
>    return MediaDataDecoder::DecodePromise::CreateAndReject(

superflous MediaDataDecoder::
Attachment #8846968 - Flags: review?(jyavenard) → review+
Comment on attachment 8846969 [details]
Bug 1315850 - Implement video decoding through CDM.

https://reviewboard.mozilla.org/r/119998/#review121962

::: dom/media/gmp/ChromiumCDMChild.h:106
(Diff revision 1)
>    void DecryptFailed(uint32_t aId, cdm::Status aStatus);
>  
>    GMPContentChild* mPlugin = nullptr;
>    cdm::ContentDecryptionModule_8* mCDM = nullptr;
>  
> +  nsDataHashtable<nsUint64HashKey, uint64_t> mFrameDurations;

We have made a DurationMap in PlatformDecoderModule which is far less heavy than nsDataHashTable and due to the typical length of the hash (around 4) is certainly going to be quicker.

can't we use that instead?

::: dom/media/gmp/ChromiumCDMChild.cpp:403
(Diff revision 1)
>  ChromiumCDMChild::RecvDecryptAndDecodeFrame(const CDMInputBuffer& aBuffer)
>  {
>    GMP_LOG("ChromiumCDMChild::RecvDecryptAndDecodeFrame()");
>    MOZ_ASSERT(mDecoderInitialized);
> +
> +  // We may not get the same out of the CDM decoder as we put in, and there

the CDM doesn't output in presentation order?

that would be rather surprising...

::: dom/media/gmp/ChromiumCDMParent.h:64
(Diff revision 1)
>  
>    // TODO: Add functions to for clients to send data to CDM, and
>    // a Close() function.
> -  RefPtr<MediaDataDecoder::InitPromise> InitializeVideoDecoder(const gmp::CDMVideoDecoderConfig& aConfig);
> +  RefPtr<MediaDataDecoder::InitPromise> InitializeVideoDecoder(
> +    const gmp::CDMVideoDecoderConfig& aConfig,
> +    VideoInfo aInfo,

const VideoInfo& to avoid unecessary copy

::: dom/media/gmp/ChromiumCDMParent.cpp:427
(Diff revision 1)
> +  b.mPlanes[2].mOffset = aFrame.mVPlane().mPlaneOffset();
> +  b.mPlanes[2].mSkip = 0;
> +
> +  gfx::IntRect pictureRegion(0, 0, aFrame.mImageWidth(), aFrame.mImageHeight());
> +  RefPtr<VideoData> v =
> +    VideoData::CreateAndCopyData(mVideoInfo,

i think this will no longer compile as CreateAndCopyData prototype has changed (or will change very soon)

::: dom/media/gmp/ChromiumCDMParent.cpp:441
(Diff revision 1)
> +
> +  RefPtr<ChromiumCDMParent> self = this;
> +  if (v) {
> +    nsTArray<RefPtr<MediaData>> output;
> +    output.AppendElement(Move(v));
> +    mDecodePromise.ResolveIfExists(Move(output), __func__);

you could do without the temporary array.
{ Move(v) }

::: dom/media/platforms/agnostic/eme/ChromiumCDMVideoDecoder.cpp:78
(Diff revision 1)
>  
>    RefPtr<gmp::ChromiumCDMParent> cdm = mCDMParent;
> +  VideoInfo info = mConfig;
> +  RefPtr<layers::ImageContainer> imageContainer = mImageContainer;
>    return InvokeAsync(mGMPThread, __func__,
> -    [cdm, config]() {
> +    [cdm, config, info, imageContainer]() {

you could pass self instead , but doesn't matter much
Attachment #8846969 - Flags: review?(jyavenard) → review+
Comment on attachment 8846970 [details]
Bug 1315850 - Implement CDM video decoder flush.

https://reviewboard.mozilla.org/r/120000/#review121964

::: dom/media/gmp/ChromiumCDMParent.cpp:543
(Diff revision 1)
> +}
> +
> +ipc::IPCResult
> +ChromiumCDMParent::RecvResetVideoDecoderComplete()
> +{
> +  mFlushDecoderPromise.ResolveIfExists(true, __func__);

that should be Resolve() as it would be a logic error if the flush promise didn't exist at this time
Attachment #8846970 - Flags: review?(jyavenard) → review+
Comment on attachment 8846971 [details]
Bug 1315850 - Implement CDM video decoder drain.

https://reviewboard.mozilla.org/r/120002/#review121966

::: dom/media/gmp/ChromiumCDMChild.h:10
(Diff revision 1)
>  
>  #ifndef ChromiumCDMChild_h_
>  #define ChromiumCDMChild_h_
>  
>  #include "mozilla/gmp/PChromiumCDMChild.h"
> +#include "WidevineVideoFrame.h"

alphabetical order

::: dom/media/gmp/ChromiumCDMChild.h:104
(Diff revision 1)
>    mozilla::ipc::IPCResult RecvDecryptAndDecodeFrame(const CDMInputBuffer& aBuffer) override;
> +  mozilla::ipc::IPCResult RecvDrain() override;
>    mozilla::ipc::IPCResult RecvDestroy() override;
>  
>    void DecryptFailed(uint32_t aId, cdm::Status aStatus);
> +  void ReturnOutput(WidevineVideoFrame& aFrame);

const ?

::: dom/media/gmp/ChromiumCDMChild.cpp:439
(Diff revision 1)
> -
> -    gmp::CDMVideoFrame output;
> +  gmp::CDMVideoFrame output;
> -    output.mFormat() = static_cast<cdm::VideoFormat>(frame.Format());
> -    output.mImageWidth() = frame.Size().width;
> -    output.mImageHeight() = frame.Size().height;
> -    output.mData() = Move(reinterpret_cast<WidevineBuffer*>(frame.FrameBuffer())->ExtractBuffer());
> +  output.mFormat() = static_cast<cdm::VideoFormat>(aFrame.Format());
> +  output.mImageWidth() = aFrame.Size().width;
> +  output.mImageHeight() = aFrame.Size().height;
> +  output.mData() = Move(reinterpret_cast<WidevineBuffer*>(aFrame.FrameBuffer())->ExtractBuffer());

booooo

::: dom/media/gmp/ChromiumCDMChild.cpp:458
(Diff revision 1)
> +mozilla::ipc::IPCResult
> +ChromiumCDMChild::RecvDrain()
> +{
> +  WidevineVideoFrame frame;
> +  cdm::InputBuffer sample;
> +  cdm::Status rv = mCDM->DecryptAndDecodeFrame(sample, &frame);

how does that work?

DecrypAndDecoder if given an empty buffer will return an earlier sample?

I guess it works because the MFR will call continuously Drain until an empty array is returned.
Attachment #8846971 - Flags: review?(jyavenard) → review+
Comment on attachment 8846972 [details]
Bug 1315850 - Shutdown CDMVideoDecoder.

https://reviewboard.mozilla.org/r/120004/#review121968

::: dom/media/gmp/ChromiumCDMParent.cpp:570
(Diff revision 1)
>  
> +RefPtr<ShutdownPromise>
> +ChromiumCDMParent::ShutdownVideoDecoder()
> +{
> +  mInitVideoDecoderPromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
> +  mDecodePromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);

it would be a logic error that mDecodePromise is set. Flush is always called prior a shutdown.

Please assert that mDecoderPromise is empty.

::: dom/media/gmp/ChromiumCDMParent.cpp:573
(Diff revision 1)
> +{
> +  mInitVideoDecoderPromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
> +  mDecodePromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
> +  MOZ_ASSERT(mFlushDecoderPromise.IsEmpty());
> +  if (!SendDeinitializeVideoDecoder()) {
> +    return ShutdownPromise::CreateAndReject(false, __func__);

should always resolve the shutdown promise. We have assertions everywhere that a shutdown promise is only ever resolved. it can't be rejected.
Attachment #8846972 - Flags: review?(jyavenard) → review+
Comment on attachment 8846947 [details]
Bug 1315850 - Add media.eme.chromium-api.enabled to ContentPrefs.cpp.

https://reviewboard.mozilla.org/r/119954/#review121970
Attachment #8846947 - Flags: review?(bugs) → review+
Comment on attachment 8846949 [details]
Bug 1315850 - Add LaunchGMPForNodeId to allowed sync IPCs.

I'm not an IPC peer.
Attachment #8846949 - Flags: review?(bugs) → review?(kchen)
But what is the followup which makes the operation async?
Comment on attachment 8846948 [details]
Bug 1315850 - Add GMPService::GetContentChild() with unresolved NodeId.

https://reviewboard.mozilla.org/r/119956/#review122270

r+ with nit & concern:

::: dom/media/gmp/GMPServiceParent.cpp:411
(Diff revision 1)
> +                          aNodeId.mGMPName,
> +                          nodeId);
> +  if (NS_FAILED(rv)) {
> +    return GetGMPContentParentPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
> +  }
> +  return GetContentParent(aHelper, nodeId, aAPI, aTags);

Would you mind renaming 'nodeId' to 'nodeIdString' in this function?
It would match nicely with the other GetContentParent's parameter name, and would make this call a bit less confusingly recursive-looking!

::: dom/media/gmp/GMPTypes.ipdlh:13
(Diff revision 1)
> +  nsString mOrigin;
> +  nsString mTopLevelOrigin;

Do we really want to give this kind of data to the GMP, from a privacy point of view? (Or is it also going away later on, once decryptor&decoder are tightly coupled?)
Attachment #8846948 - Flags: review?(gsquelart) → review+
Comment on attachment 8846951 [details]
Bug 1315850 - Add GMP_LOG macro.

https://reviewboard.mozilla.org/r/119962/#review122302

::: dom/media/gmp/GMPLog.h:13
(Diff revision 1)
> +
> +#include "mozilla/Logging.h"
> +
> +namespace mozilla {
> +
> +extern LogModule* GetGMPLog();

This would be a great opportunity to move the implementation of GetGMPLog() to GMPLog.cpp.

But it would be a lot of work to make all existing uses point to this one, so we can do that in another bug...
Attachment #8846951 - Flags: review?(gsquelart) → review+
Comment on attachment 8846952 [details]
Bug 1315850 - Add ChromiumAdapter which we can use instead of WidevineAdapter.

https://reviewboard.mozilla.org/r/119964/#review122384

::: dom/media/gmp/ChromiumCDMAdapter.h:17
(Diff revision 1)
>  
>  struct GMPPlatformAPI;
>  
>  namespace mozilla {
>  
> -class WidevineAdapter : public gmp::GMPAdapter {
> +class ChromiumCDMAdapter : public gmp::GMPAdapter {

'{' on separate line. (At least in the new file)

::: dom/media/gmp/ChromiumCDMAdapter.cpp:79
(Diff revision 1)
>               kEMEKeySystemWidevine.get(),
>               kEMEKeySystemWidevine.Length(),

Just to verify: Same key system as Widevine?
Attachment #8846952 - Flags: review?(gsquelart) → review+
Comment on attachment 8846949 [details]
Bug 1315850 - Add LaunchGMPForNodeId to allowed sync IPCs.

https://reviewboard.mozilla.org/r/119958/#review122456

::: commit-message-dcd53:3
(Diff revision 1)
> +This merges two existing off-main-thread sync IPCs into a single operation.  We
> +will change them into a single async operation in a follow up.

Which two sync IPCs were merged? Should they be removed from the list?

What's the plan to make it async?
Attachment #8846949 - Flags: review?(kchen)
Comment on attachment 8846949 [details]
Bug 1315850 - Add LaunchGMPForNodeId to allowed sync IPCs.

https://reviewboard.mozilla.org/r/119958/#review122658

::: commit-message-dcd53:3
(Diff revision 1)
> +This merges two existing off-main-thread sync IPCs into a single operation.  We
> +will change them into a single async operation in a follow up.

This patch adds a new IPC which does what PGMPService::LaunchGMP and PGMPService::GetGMPNodeId does in a single message  PGMPService::LaunchGMPForNodeId.

Note these IPCs all run off main thread. So they can't cause jank.

I will be converting the IPCs involved here to be async messages in bug 1337080.

I am rewriting the GMP IPC code. Given that I am going to be removing old code, it doesn't make sense to change these messages to be async now, as I'll be throwing the calling code away after we've switched over to the new code.

I can remove PGMPService::LaunchGMP and PGMPService::GetGMPNodeId once I've switched GMPs over to the new protocol.
Attachment #8846949 - Flags: review?(kchen)
(In reply to Olli Pettay [:smaug] from comment #45)
> But what is the followup which makes the operation async?

bug 1337080. These IPCs do not run on the main thread, so they do not cause UI jank.
Comment on attachment 8846952 [details]
Bug 1315850 - Add ChromiumAdapter which we can use instead of WidevineAdapter.

https://reviewboard.mozilla.org/r/119964/#review122702

::: dom/media/gmp/GMPContentChild.cpp:167
(Diff revision 1)
>  
>  mozilla::ipc::IPCResult
>  GMPContentChild::RecvPChromiumCDMConstructor(PChromiumCDMChild* aActor)
>  {
> -  // TODO: Implement.
> +  ChromiumCDMChild* child = static_cast<ChromiumCDMChild*>(aActor);
> +  cdm::Host_8* host = child;

This won't compile until the next patch, I think.
Could you just swap the two patches? If not, it's not that important.
Comment on attachment 8846953 [details]
Bug 1315850 - Implement trivial cdm::Host functions.

https://reviewboard.mozilla.org/r/119966/#review122732

Keeping 'r?' for now, to re-review updated code (unless you show me I'm wrong, or course).

::: dom/media/gmp/ChromiumCDMChild.cpp:111
(Diff revision 1)
> +    nsTArray<uint8_t> kid;
> +    kid.AppendElements(key.key_id, key.key_id_size);
> +    keys.AppendElement(CDMKeyInformation(kid, key.status, key.system_code));
> +  }

Unless I'm missing something, this looks dangerous!

1. `kid` is an `nsTArray`, its elements are stored in a buffer it owns on the heap.
2. Bytes pointed at by `key` are copied into the buffer on the heap.
3. `CDMKeyInformation`'s first member is `uint8_t[] mKeyId`, which is effectively a `uint8_t*` (right?), so we are giving it a pointer to the buffer owned by `kid`. The temporary `CDMKeyInformation` is copied into `keys`, still pointing to the same buffer.
4. At the end of the block, `kid` is destroyed, so the buffer is freed, but `keys` still points to it after that.

I think instead you'll want to pass the pointer from `aKeysInfo[i].key_id` directly to the `CDMKeyInformation`.
And if key_id_size can vary, you'll want to store that somewhere too; E.g.: Maybe `CDMKeyInformation::mKeyId` should be an `nsTArray<uint8_t>`?
(Waiting to see the `RecvOnSessionKeysChange` code to see what's happening on the other side...)
Comment on attachment 8846954 [details]
Bug 1315850 - Add GetGMPAbstractThread() to GMPUtils.h.

https://reviewboard.mozilla.org/r/119968/#review122742

r+ with nullptr-check if needed.

::: dom/media/gmp/GMPUtils.cpp:237
(Diff revision 1)
> +  RefPtr<gmp::GeckoMediaPluginService> service =
> +    gmp::GeckoMediaPluginService::GetGeckoMediaPluginService();
> +  return service->GetAbstractGMPThread();

In other places, we check for nullptr before using the service, shouldn't you do the same here?
Attachment #8846954 - Flags: review?(gsquelart) → review+
Comment on attachment 8846955 [details]
Bug 1315850 - Stub out ChromiumCDMProxy.

https://reviewboard.mozilla.org/r/119970/#review122752

::: dom/media/gmp/ChromiumCDMProxy.h:19
(Diff revision 1)
> +namespace mozilla {
> +
> +class MediaRawData;
> +class DecryptJob;
> +
> +class ChromiumCDMProxy : public CDMProxy {

'{' on new line.
Attachment #8846955 - Flags: review?(gsquelart) → review+
Comment on attachment 8846956 [details]
Bug 1315850 - Create ChromiumCDMProxy in MediaKeys.cpp when preffed on.

https://reviewboard.mozilla.org/r/119972/#review122786
Attachment #8846956 - Flags: review?(gsquelart) → review+
Comment on attachment 8846949 [details]
Bug 1315850 - Add LaunchGMPForNodeId to allowed sync IPCs.

https://reviewboard.mozilla.org/r/119958/#review122788
Attachment #8846949 - Flags: review?(kchen) → review+
Comment on attachment 8846957 [details]
Bug 1315850 - Ensure we query for the correct string in HavePluginForKeySystem.

https://reviewboard.mozilla.org/r/119974/#review122792

::: dom/media/eme/MediaKeySystemAccess.cpp:102
(Diff revision 1)
> -  bool havePlugin = HaveGMPFor(NS_LITERAL_CSTRING(GMP_API_DECRYPTOR),
> -                               { aKeySystem });
> +  nsCString api = MediaPrefs::EMEChromiumAPIEnabled()
> +    ? NS_LITERAL_CSTRING(CHROMIUM_CDM_API)
> +    : NS_LITERAL_CSTRING(GMP_API_DECRYPTOR);
> +
> +  bool havePlugin = HaveGMPFor(api, { aKeySystem });

opt nit: Do the ternary test right inside HaveGMPFor(), so we don't create an nsCString that (I think) makes a copy of the literal string.
Or if you prefer the clarity of a separate variable, try making it `const auto&`, which should also spare us an nsCString.
Attachment #8846957 - Flags: review?(gsquelart) → review+
Comment on attachment 8846958 [details]
Bug 1315850 - Implement ChromiumCDMProxy initialization.

https://reviewboard.mozilla.org/r/119976/#review122796

::: dom/media/gmp/ChromiumCDMParent.h:31
(Diff revision 1)
>  public:
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ChromiumCDMParent)
>  
> -  explicit ChromiumCDMParent(GMPContentParent *aContentParent);
> +  ChromiumCDMParent(GMPContentParent *aContentParent, uint32_t aPluginId);
> +
> +  uint32_t GetPluginId() const { return mPluginId; }

I think 'Get' is supposed to indicate functions that may return an invalid value that needs to be checked for (e.g.: nullptr), but in this case the plugin id should always be valid.

::: dom/media/gmp/ChromiumCDMParent.h:72
(Diff revision 1)
>    ipc::IPCResult RecvShutdown() override;
>    void ActorDestroy(ActorDestroyReason aWhy) override;
>  
> +  const uint32_t mPluginId;
>    GMPContentParent* mContentParent;
> +  ChromiumCDMProxy* mProxy; // Non-owning reference!

Strictly speaking it's a pointer, not a reference. :-)

Given all the owning pointer types (RefPtr, UniquePtr, etc.), I think a raw pointer now always means non-owning by default.
Is there something special about this pointer, that you think is somewhat important to highlight the non-ownership?

::: dom/media/gmp/ChromiumCDMParent.cpp:15
(Diff revision 1)
> -ChromiumCDMParent::ChromiumCDMParent(GMPContentParent *aContentParent)
> -  : mContentParent(aContentParent)
> +ChromiumCDMParent::ChromiumCDMParent(GMPContentParent *aContentParent,
> +                                     uint32_t aPluginId)
> +  : mPluginId(aPluginId)
> +  , mContentParent(aContentParent)
>  {

You are not initializing mProxy to nullptr, are you confident there is no risk of any use happening before Init is called?
Attachment #8846958 - Flags: review?(gsquelart) → review+
Comment on attachment 8846959 [details]
Bug 1315850 - Handle sending and receiving key session messages.

https://reviewboard.mozilla.org/r/119978/#review122814

::: dom/media/gmp/ChromiumCDMParent.cpp:128
(Diff revision 1)
> +  RefPtr<Runnable> task = NewRunnableMethod<uint32_t, nsresult, nsCString>(
> +    mProxy, &ChromiumCDMProxy::RejectPromise, aPromiseId, aError, aErrorMessage);
> +  NS_DispatchToMainThread(task);

In case you don't know: You can just do `NS_DispatchToMainThread(NewRunnableMethod<...>(...));

I think I saw a few other split tasks&dispatches in previous patches, if you want to simplify them too.

::: dom/media/gmp/ChromiumCDMParent.cpp:167
(Diff revision 1)
> +  NS_DispatchToMainThread(
> +    NS_NewRunnableFunction([proxy, sid, messageType, msg] () mutable
> +  {
> +    proxy->OnSessionMessage(sid, messageType, msg);
> +  })
> +  );

Function body should be indented.
Also, final ');' could go at the end of the previous line.

::: dom/media/gmp/ChromiumCDMParent.cpp:167
(Diff revision 1)
> +  NS_DispatchToMainThread(
> +    NS_NewRunnableFunction([proxy, sid, messageType, msg] () mutable

Oh, you knew! :-)

::: dom/media/gmp/ChromiumCDMParent.cpp:225
(Diff revision 1)
> +  RefPtr<Runnable> task = NewRunnableMethod<nsString, UnixTime>(
> +    mProxy, &ChromiumCDMProxy::OnExpirationChange,
> +    NS_ConvertUTF8toUTF16(aSessionId), int64_t(aSecondsSinceEpoch * 1000));

Rant: Could we please make physical units more obvious in type names? (This could be the subject of a separate bug.)

From this code, I'm guessing `UnixTime` is in ms.

Also, confusingly ChromiumCDMProxy::OnExpirationChange takes a GMPTimestamp, not a UnixTime -- but they're both typedefs of int64_t, so that compiles. You may still want to use GMPTimestamp here.
Attachment #8846959 - Flags: review?(gsquelart) → review+
Comment on attachment 8846960 [details]
Bug 1315850 - Connect MediaKeys.createSession to Chromium CDM.

https://reviewboard.mozilla.org/r/119980/#review122850
Attachment #8846960 - Flags: review?(gsquelart) → review+
Comment on attachment 8846961 [details]
Bug 1315850 - Add more logging to Chromium CDM actors.

https://reviewboard.mozilla.org/r/119982/#review122854

::: dom/media/gmp/ChromiumCDMChild.cpp:41
(Diff revision 1)
>  }
>  
>  cdm::Buffer*
>  ChromiumCDMChild::Allocate(uint32_t aCapacity)
>  {
> +  GMP_LOG("ChromiumCDMChild::Allocate(capacity=%u)", aCapacity);

For a uint32_t, officially you need to use PRIu32 -- though I think on all current platforms uint32_t==unsigned, so it should work anyway.
(Repeated below)
Attachment #8846961 - Flags: review?(gsquelart) → review+
Comment on attachment 8846945 [details]
Bug 1315850 - Add PChromiumCDM.ipdl for Widevine CDM.

https://reviewboard.mozilla.org/r/119950/#review122874

Extra nit:

::: dom/media/gmp/ChromiumCDMChild.h:22
(Diff revision 1)
> +protected:
> +  ~ChromiumCDMChild() {}
> +
> +  mozilla::ipc::IPCResult RecvInit(const bool& aAllowDistinctiveIdentifier,
> +                                   const bool& aAllowPersistentState) override;

Virtual methods of publicly-inherited PChromiumCDMChild are public, so overrides should also be public.
(See isocpp faq: https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public , or Liskov Substitution Principle).

Same for ChromiumCDMParent.

Though it seems other gmp classes have done that before, so I guess the trend is there, if you want to claim consistency!
Comment on attachment 8846962 [details]
Bug 1315850 - Send decrypt operations to Chromium CDM.

https://reviewboard.mozilla.org/r/119984/#review123186

::: dom/media/gmp/ChromiumCDMChild.cpp:344
(Diff revision 1)
> +    return IPC_OK();
> +  }
> +
> +  if (!output.DecryptedBuffer() ||
> +      output.DecryptedBuffer()->Size() != aBuffer.mData().Length()) {
> +    // We input and output should exactly match.

'We' -> 'The sizes of'?

::: dom/media/gmp/ChromiumCDMParent.cpp:111
(Diff revision 1)
> +    GMP_LOG("InitCDMInputBuffer clear/cipher subsamples don't match");
> +    return false;
> +  }
> +
> +  nsTArray<uint8_t> data;
> +  data.AppendElements(aSample->Data(), aSample->Size());

Wouldn't it be nice to make all these array manipulations fallible?
(But I'm guessing the amount of data should be relatively small, so if this operation fails, Firefox as a whole is probably in trouble anyway?)
Attachment #8846962 - Flags: review?(gsquelart) → review+
Comment on attachment 8846963 [details]
Bug 1315850 - Ensure GMPParent checks whether the adapter version is present.

https://reviewboard.mozilla.org/r/119986/#review123226

::: commit-message-de17c:1
(Diff revision 1)
> +Bug 1315850 - Ensure GMPParent checks the whether the adapter version is present. r?gerald

Remove 'the' before 'whether'
Attachment #8846963 - Flags: review?(gsquelart) → review+
Comment on attachment 8846964 [details]
Bug 1315850 - Add CDMProxy::AsChromiumCDMProxy().

https://reviewboard.mozilla.org/r/119988/#review123230
Attachment #8846964 - Flags: review?(gsquelart) → review+
Comment on attachment 8846967 [details]
Bug 1315850 - Add threadsafe ChromiumCDMProxy::GetCDMParent.

https://reviewboard.mozilla.org/r/119994/#review123232

::: commit-message-5ad7b:10
(Diff revision 1)
> +task queue in order to get a reference to the ChromiumCDMParent so that it can
> +talk to the CDM (on the GMP thread).
> +
> +Additionally we'll need to shutdown the ChromiumCDMProxy, and if we do that
> +on the main threrad while the ChromiumCDMVideoDecoder is trying to get the
> +ChromiumCDMParent reference, we could hit threadsafety issues.

Add space between 'thread' and 'safety' -- Or is that 1337-speak?

::: dom/media/gmp/ChromiumCDMProxy.cpp:27
(Diff revision 1)
>               aKeySystem,
>               aDistinctiveIdentifierRequired,
>               aPersistentStateRequired,
>               aMainThread)
>    , mCrashHelper(aCrashHelper)
> -  , mCDM(nullptr)
> +  , mCDMMutex("ChromiumCDMProxy")

Shouldn't you keep the `mCDM(nullptr)` initialization? (Unless you trust Init will happen before anything else)
Attachment #8846967 - Flags: review?(gsquelart) → review+
Comment on attachment 8846973 [details]
Bug 1315850 - Shutdown ChromiumCDMParent.

https://reviewboard.mozilla.org/r/120006/#review123236

::: dom/media/gmp/ChromiumCDMParent.cpp:534
(Diff revision 1)
> +    // Plugin crash.
> +    MOZ_ASSERT(aWhy == AbnormalShutdown);
> +    Shutdown();
> +  }
> +  MOZ_ASSERT(mIsShutdown);
> +  RefPtr<ChromiumCDMParent> kungfuDeathGrip(this);

'kungFuDeathGrip' (capital F) -- Though I'm not sure the name matters!?
Attachment #8846973 - Flags: review?(gsquelart) → review+
Comment on attachment 8846975 [details]
Bug 1315850 - Implement CDM persistent sessions.

https://reviewboard.mozilla.org/r/120010/#review123242

::: dom/media/gmp/ChromiumCDMParent.cpp:240
(Diff revision 1)
> +          this, aPromiseId, aSuccessful);
> +  if (!mProxy || mIsShutdown) {
> +    return IPC_OK();
> +  }
> +
> +  RefPtr<Runnable> task = NewRunnableMethod<uint32_t,bool>(

Add space before 'bool'
Attachment #8846975 - Flags: review?(gsquelart) → review+
Comment on attachment 8846976 [details]
Bug 1315850 - Rename DetailedPromise Status enum.

https://reviewboard.mozilla.org/r/120012/#review123246
Attachment #8846976 - Flags: review?(gsquelart) → review+
Comment on attachment 8846977 [details]
Bug 1315850 - Port the work around from Bug 1343140 to the new CDM video decoder architecture.

https://reviewboard.mozilla.org/r/120022/#review123248

::: dom/media/gmp/ChromiumCDMChild.cpp:453
(Diff revision 1)
>    cdm::Status rv = mCDM->DecryptAndDecodeFrame(input, &frame);
>    GMP_LOG("WidevineVideoDecoder::Decode(timestamp=%" PRId64 ") rv=%d",
>            input.timestamp, rv);
>  
> -  if (rv == cdm::kSuccess) {
> +  switch (rv) {
> +    case cdm::kNoKey: {

You don't strictly need '{}' blocks in any of these cases.
Attachment #8846977 - Flags: review?(gsquelart) → review+
Comment on attachment 8846945 [details]
Bug 1315850 - Add PChromiumCDM.ipdl for Widevine CDM.

https://reviewboard.mozilla.org/r/119950/#review122874

> Virtual methods of publicly-inherited PChromiumCDMChild are public, so overrides should also be public.
> (See isocpp faq: https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public , or Liskov Substitution Principle).
> 
> Same for ChromiumCDMParent.
> 
> Though it seems other gmp classes have done that before, so I guess the trend is there, if you want to claim consistency!

As discussed on IRC, these methods are protected in the base class.
Comment on attachment 8846945 [details]
Bug 1315850 - Add PChromiumCDM.ipdl for Widevine CDM.

https://reviewboard.mozilla.org/r/119950/#review121846

> A bit strange to see 'protected' in a 'final' class, but I guess it's consistent with the child, so it's fine if you prefer that.

Yes, but the overriden methods are protected.

> According to the C++ standards, adjacent underscores are reserved to the compiler&library implementation.
> But I see that this __delete__ has already been used all over the place, so it's probably fine to continue with it... Separate bug?

Indeed, it's IPDL convention to use __delete__ to delete. See https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Tutorial .
Comment on attachment 8846948 [details]
Bug 1315850 - Add GMPService::GetContentChild() with unresolved NodeId.

https://reviewboard.mozilla.org/r/119956/#review122270

> Would you mind renaming 'nodeId' to 'nodeIdString' in this function?
> It would match nicely with the other GetContentParent's parameter name, and would make this call a bit less confusingly recursive-looking!

Done.

> Do we really want to give this kind of data to the GMP, from a privacy point of view? (Or is it also going away later on, once decryptor&decoder are tightly coupled?)

No, we certainly do not want the GMP to see the origin strings. This struct is only passed between the content and main processes, so the GMP will never see it. The GMP process only sees the node ID as the random bytes we generate for the origins.
Comment on attachment 8846952 [details]
Bug 1315850 - Add ChromiumAdapter which we can use instead of WidevineAdapter.

https://reviewboard.mozilla.org/r/119964/#review122384

> Just to verify: Same key system as Widevine?

Yup, the new CDM harness uses the same keysystem string, as that's set by Widevine, not by us.
Comment on attachment 8846952 [details]
Bug 1315850 - Add ChromiumAdapter which we can use instead of WidevineAdapter.

https://reviewboard.mozilla.org/r/119964/#review122702

> This won't compile until the next patch, I think.
> Could you just swap the two patches? If not, it's not that important.

Yeah, thihs doesn't compile until the next patch, but swapping them doesn't compile either, so I'll just leave them as ordered.
Comment on attachment 8846953 [details]
Bug 1315850 - Implement trivial cdm::Host functions.

https://reviewboard.mozilla.org/r/119966/#review122732

> Unless I'm missing something, this looks dangerous!
> 
> 1. `kid` is an `nsTArray`, its elements are stored in a buffer it owns on the heap.
> 2. Bytes pointed at by `key` are copied into the buffer on the heap.
> 3. `CDMKeyInformation`'s first member is `uint8_t[] mKeyId`, which is effectively a `uint8_t*` (right?), so we are giving it a pointer to the buffer owned by `kid`. The temporary `CDMKeyInformation` is copied into `keys`, still pointing to the same buffer.
> 4. At the end of the block, `kid` is destroyed, so the buffer is freed, but `keys` still points to it after that.
> 
> I think instead you'll want to pass the pointer from `aKeysInfo[i].key_id` directly to the `CDMKeyInformation`.
> And if key_id_size can vary, you'll want to store that somewhere too; E.g.: Maybe `CDMKeyInformation::mKeyId` should be an `nsTArray<uint8_t>`?
> (Waiting to see the `RecvOnSessionKeysChange` code to see what's happening on the other side...)

An uint8_t[] in IPDL gets converted to an nsTArray<uint8_t>. Through the wonders of VisualAssist I can see that the generated CDMKeyInformation struct contains an nsTArray<uint8_t>, and we're assigning (i.e. copying) the kid into it. So we're good here.
Comment on attachment 8846957 [details]
Bug 1315850 - Ensure we query for the correct string in HavePluginForKeySystem.

https://reviewboard.mozilla.org/r/119974/#review122792

> opt nit: Do the ternary test right inside HaveGMPFor(), so we don't create an nsCString that (I think) makes a copy of the literal string.
> Or if you prefer the clarity of a separate variable, try making it `const auto&`, which should also spare us an nsCString.

We use HaveGMPFor() elsewhere with a different string, so const auto& it is.
Comment on attachment 8846958 [details]
Bug 1315850 - Implement ChromiumCDMProxy initialization.

https://reviewboard.mozilla.org/r/119976/#review122796

> Strictly speaking it's a pointer, not a reference. :-)
> 
> Given all the owning pointer types (RefPtr, UniquePtr, etc.), I think a raw pointer now always means non-owning by default.
> Is there something special about this pointer, that you think is somewhat important to highlight the non-ownership?

What's special about this pointer is that it's non-owning because otherwise it would create a referencec cycle. I should just say that instead...

> You are not initializing mProxy to nullptr, are you confident there is no risk of any use happening before Init is called?

I'll just initialize it to nullptr to be safe. Thanks!
Comment on attachment 8846959 [details]
Bug 1315850 - Handle sending and receiving key session messages.

https://reviewboard.mozilla.org/r/119978/#review122814

> In case you don't know: You can just do `NS_DispatchToMainThread(NewRunnableMethod<...>(...));
> 
> I think I saw a few other split tasks&dispatches in previous patches, if you want to simplify them too.

I'll switch to the single line method. Thanks for pointing that out.

> Function body should be indented.
> Also, final ');' could go at the end of the previous line.

I'm going to just clang-format everything, which will catch all these.

> Oh, you knew! :-)

I was just testing to see if you'd be consistent. Honest!

> Rant: Could we please make physical units more obvious in type names? (This could be the subject of a separate bug.)
> 
> From this code, I'm guessing `UnixTime` is in ms.
> 
> Also, confusingly ChromiumCDMProxy::OnExpirationChange takes a GMPTimestamp, not a UnixTime -- but they're both typedefs of int64_t, so that compiles. You may still want to use GMPTimestamp here.

ChromiumCDMProxy::OnExpirationChange implements a virtual function that was designed to work with the GMPDecryptor API, which this bug moves us a large way towards deprecating. Once we've irradicated GMPDecryptor, we'll be well placed to tidy this up.

Also note here however that aSecondsSinceEpoch has to be a type that is compatible with IPDL, as this is a receive message function.

I'll cast the value passed into the runnable to a GMPTimestamp rather than an int64_t at least.
Comment on attachment 8846962 [details]
Bug 1315850 - Send decrypt operations to Chromium CDM.

https://reviewboard.mozilla.org/r/119984/#review123186

> Wouldn't it be nice to make all these array manipulations fallible?
> (But I'm guessing the amount of data should be relatively small, so if this operation fails, Firefox as a whole is probably in trouble anyway?)

Basically yes to the latter, if Firefox fails on an array operation here, we've got bigger problems!

I'm going to put these into shmems in future.
Comment on attachment 8846966 [details]
Bug 1315850 - Create CDM video decoder in EMEDecoderModule.

https://reviewboard.mozilla.org/r/119992/#review121958

> this assumes the ChromiumCDMVideoDecoder can decode all video codec...
> 
> which I know is true, but can't we have a way to abstract that and bubble up that information rather than having those assumptions?
> 
> seems like we're limiting ourselves to what kind of CDM we will support in the future.

The SupportsMimeType call on line 351 checks with the GMPService whether we have a CDM that can handle that mime type. The GMPService's understanding of what is playable is populated from the list of supported codecs in the CDM's manifest.json file. So we should be limiting ourselves to codecs that the CDM advertised that it can play.
Comment on attachment 8846967 [details]
Bug 1315850 - Add threadsafe ChromiumCDMProxy::GetCDMParent.

https://reviewboard.mozilla.org/r/119994/#review123232

> Shouldn't you keep the `mCDM(nullptr)` initialization? (Unless you trust Init will happen before anything else)

mCDM is actually a RefPtr<gmp::ChromiumCDMParent>, so its constructor already initializes it. I didn't need to be initializing it to null in the first place.
Comment on attachment 8846968 [details]
Bug 1315850 - Initialize video decoder.

https://reviewboard.mozilla.org/r/119996/#review121960

> can we have some assertions indicating on which target thread everything is running ?

All IPC traffic arrives onthe IPC main message loop. All outgoing needs to be on that thread too. We can assert that, as it's useful to ensure that the CDM is calling us on the thread we call it on.

> under which circumstances will RecvDeinitalizeVideoDecoder be received?
> 
> is that called before shutting down?
> 
> I'm not sure of the use of a mDecoderInitialized member tbh. If init failed, then none of those methods will ever be called. it's a strong given.
> 
> Adding even more assertion just muddy things imho.

RecvDeinitalizeVideoDecoder is received when the ChromiumCDMVideoDecoder/MediaDataDecoder is shutdown in the parent process. Our connection to the CDM remains open, and we will initialize a new decoder when a new ChromiumCDMVideoDecoder is created and initialized in the parent process. RecvDeinitalizeVideoDecoder does not inidicate that the child process is shutting down, as a new decoder may be created that uses the same CDM instance, but we do want to have deinited before the child process does shutdown.

The assertions ensure that we don't call init without de-initing, as the ChromiumCDMChild outlives the ChromiumCDMVideoDecoder in the content process. Indeed, a ChromiumCDMVideoDecoder may be associated with many ChromiumCDMVideoDecoder instances in the content process over the course of its life, so I want to ensure we're careful to init and de-init in the CDM process in the correct order.

> assuming this is called when Shutdown() is called, that could in theory be happening after initialisation failed or even before the initialisation completed and got interrupted.
> 
> so the assert is wrong

This is not called when ChromiumCDMVideoDecoder shuts down. It's called when the MediaKeys shuts down. The ChromiumCDMVideoDecoder are likely to have been shutdown by the time that happens, but there's no guarantee that they have been.

Shutdown is handled in later patches, and I'm careful to disconnect the message handlers to ensure that when we shutdown in-progress operations have their promises rejected and their results coming back from the CDM are ignored. See later patches for more on how this works. 

I don't see how the case you describe could happen, as there are no intr messages in the new protocol, and so the messages are processed in the order in which they arrive, and in general they return their results in an async IPC after calling into the CDM synchronously.

> MediaDataDecoder:: is unecessary, and that doesn't look like it fits in 80 columns

The MediaDataDecoder:: is necessary here as ChromiumCDMParent doesn't inherit from MediaDataDecoder; it's ChromiumCDMVideoDecoder that inherits from MediaDataDecoder.

I've run clang-format to fix the long line.
Comment on attachment 8846969 [details]
Bug 1315850 - Implement video decoding through CDM.

https://reviewboard.mozilla.org/r/119998/#review121962

> We have made a DurationMap in PlatformDecoderModule which is far less heavy than nsDataHashTable and due to the typical length of the hash (around 4) is certainly going to be quicker.
> 
> can't we use that instead?

Sure we can use SimpleMap here.

> the CDM doesn't output in presentation order?
> 
> that would be rather surprising...

The CDM outputs in presentation order. As the comment says, we need to input a run of frames before we get output, much like other decoders that we have. It would be clearer to say that the frame coming out may not have the same timestamp as the frame we put in. I'll make the comment clearer.

> i think this will no longer compile as CreateAndCopyData prototype has changed (or will change very soon)

It works today, but I'll keep an eye out, thanks.

> you could pass self instead , but doesn't matter much

Then I'd be accessing the ChromiumCDMVideoDeccoder's members on a different task queue, and I'd prefer to keep objects to one thread/taskqueue only where possible.
Comment on attachment 8846971 [details]
Bug 1315850 - Implement CDM video decoder drain.

https://reviewboard.mozilla.org/r/120002/#review121966

> const ?

I move the buffer contained inside the WidevineVideoFrame object out of it, so I don't think const would work nor be appropriate.

> booooo

You wouldn't believe how many times I fixed merge conflicts here while reworking these patches.

> how does that work?
> 
> DecrypAndDecoder if given an empty buffer will return an earlier sample?
> 
> I guess it works because the MFR will call continuously Drain until an empty array is returned.

Yup.

https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/dom/media/gmp/widevine-adapter/content_decryption_module.h#766
Comment on attachment 8846972 [details]
Bug 1315850 - Shutdown CDMVideoDecoder.

https://reviewboard.mozilla.org/r/120004/#review121968

> should always resolve the shutdown promise. We have assertions everywhere that a shutdown promise is only ever resolved. it can't be rejected.

OK. Thanks for pointing that out.
With review comments addressed, and enabled:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58baa7d4b6444702f271f094f0d6d8c05966f300&selectedJob=85505890

There's a leak on Linux x64 Debug non-e10s media mochitests, which I'm investigating:

[task 2017-03-14T03:53:58.241128Z] 03:53:58     INFO -      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
[task 2017-03-14T03:53:58.241854Z] 03:53:58     INFO -      |                                      | Per-Inst   Leaked|   Total      Rem|
[task 2017-03-14T03:53:58.242627Z] 03:53:58     INFO -    0 |TOTAL                                 |       23     4208|60148434       40|
[task 2017-03-14T03:53:58.243939Z] 03:53:58     INFO -  105 |CancelableRunnable                    |       48       48|  746060        1|
[task 2017-03-14T03:53:58.244666Z] 03:53:58     INFO -  151 |CondVar                               |       40      160|   26373        4|
[task 2017-03-14T03:53:58.245365Z] 03:53:58     INFO -  311 |GMPParent                             |      992      992|       6        1|
[task 2017-03-14T03:53:58.246046Z] 03:53:58     INFO -  316 |GeckoMediaPluginService               |      136      136|       1        1|
[task 2017-03-14T03:53:58.246754Z] 03:53:58     INFO -  317 |GeckoMediaPluginServiceParent         |      440      440|       1        1|
[task 2017-03-14T03:53:58.247442Z] 03:53:58     INFO -  389 |IdlePeriod                            |       24       24|    5128        1|
[task 2017-03-14T03:53:58.247554Z] 03:53:58     INFO -  534 |Mutex                                 |       32      160|  441628        5|
[task 2017-03-14T03:53:58.248265Z] 03:53:58     INFO -  581 |PGMPParent                            |      768      768|       6        1|
[task 2017-03-14T03:53:58.248943Z] 03:53:58     INFO -  670 |RefCountedMonitor                     |       80       80|      19        1|
[task 2017-03-14T03:53:58.249659Z] 03:53:58     INFO -  693 |Runnable                              |       40       80| 5101338        2|
[task 2017-03-14T03:53:58.249773Z] 03:53:58     INFO -  733 |SharedMemory                          |       32       32|    2724        1|
[task 2017-03-14T03:53:58.251768Z] 03:53:58     INFO -  967 |ipc::MessageChannel                   |      416      416|      41        1|
[task 2017-03-14T03:53:58.254364Z] 03:53:58     INFO - 1313 |nsLocalFile                           |      192      384|   12192        2|
[task 2017-03-14T03:53:58.256069Z] 03:53:58     INFO - 1457 |nsStringBuffer                        |        8       64|  950807        8|
[task 2017-03-14T03:53:58.257752Z] 03:53:58     INFO - 1504 |nsTArray_base                         |        8       72|18007569        9|
[task 2017-03-14T03:53:58.259512Z] 03:53:58     INFO - 1514 |nsThread                              |      352      352|    5127        1|
Comment on attachment 8846953 [details]
Bug 1315850 - Implement trivial cdm::Host functions.

https://reviewboard.mozilla.org/r/119966/#review125172

Sorry, didn't see the `uint8_t[]` was in an IPDL. All good then!
Attachment #8846953 - Flags: review?(gsquelart) → review+
Attachment #8846949 - Flags: review?(bugs) → review+
Comment on attachment 8850750 [details]
Bug 1315850 - Ask the GMPService for the GMP thread in GMPParent::ChildTerminated.

https://reviewboard.mozilla.org/r/123274/#review125662

r+ assuming you fix the alleged race (or prove it cannot happen!)

::: commit-message-01c0f:4
(Diff revision 1)
> +Bug 1315850 - Ask the GMPService for the GMP thread in GMPParent::ChildTerminated. r?gerald
> +
> +When we shutdown the browser while the GMPService is active we can end up
> +leaking and GMPParent, GeckoMediaPluginServiceParent, and a Runnable. I tracked

Remove first 'and'

::: dom/media/gmp/GMPParent.cpp:387
(Diff revision 1)
> -    // access this without locks!  However, debug only, and primary failure
> -    // mode outside of compiler-helped TSAN is a leak.  But better would be
> -    // to use swap() under a lock.
> +  nsCOMPtr<nsIThread> gmpThread;
> +  mps->GetThread(getter_AddRefs(gmpThread));
> +  return gmpThread;

This seems racy to me:
The returned thread pointer is stored in a local nsCOMPtr, which is destroyed at the end of the function, releasing its reference.
So in the short time betweeen the end of the function and when the nsIThread* is used, that nsIThread could be destroyed elsewhere, leading to UAF.

If I'm correct, I would suggest you return either an already_AddRefed (to enforce usage in the caller) or an nsCOMPtr (to benefit from RVO in ChildTerminated()), to ensure the nsIThread stays alive in the callers' contexts.
Attachment #8850750 - Flags: review?(gsquelart) → review+
Comment on attachment 8850750 [details]
Bug 1315850 - Ask the GMPService for the GMP thread in GMPParent::ChildTerminated.

https://reviewboard.mozilla.org/r/123274/#review125662

> This seems racy to me:
> The returned thread pointer is stored in a local nsCOMPtr, which is destroyed at the end of the function, releasing its reference.
> So in the short time betweeen the end of the function and when the nsIThread* is used, that nsIThread could be destroyed elsewhere, leading to UAF.
> 
> If I'm correct, I would suggest you return either an already_AddRefed (to enforce usage in the caller) or an nsCOMPtr (to benefit from RVO in ChildTerminated()), to ensure the nsIThread stays alive in the callers' contexts.

Yup, I think you're right. I've updated GMPParent::GMPThread() and GMPContentParent::GMPThread() to return an nsCOMPtr<nsIThread>.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 34 changesets with 135 changes to 54 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://api.pub.build.mozilla.org/treestatus/trees/autoland :
remote: <urlopen error [Errno 110] Connection timed out>
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4c1af85be7
Add PChromiumCDM.ipdl for Widevine CDM. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/778ae7585a4e
Add pref to toggle on new CDM decoder backend. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/5453681637fc
Add media.eme.chromium-api.enabled to ContentPrefs.cpp. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9baeeb80d632
Add GMPService::GetContentChild() with unresolved NodeId. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/926546bcd7aa
Add LaunchGMPForNodeId to allowed sync IPCs. r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/997ee5288cd0
Add GMPService::GetCDM. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26e287c282c
Add GMP_LOG macro. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff38b37821b0
Add ChromiumAdapter which we can use instead of WidevineAdapter. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/89cad106793c
Implement trivial cdm::Host functions. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/270df6226231
Add GetGMPAbstractThread() to GMPUtils.h. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3bc63b5200
Stub out ChromiumCDMProxy. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/45dd49944a96
Create ChromiumCDMProxy in MediaKeys.cpp when preffed on. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab102cdd2848
Ensure we query for the correct string in HavePluginForKeySystem. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/63434c1f9900
Implement ChromiumCDMProxy initialization. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1281784ba5b
Handle sending and receiving key session messages. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d68905b4ec
Connect MediaKeys.createSession to Chromium CDM. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c077aafe07
Add more logging to Chromium CDM actors. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/e46942398895
Send decrypt operations to Chromium CDM. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/a872ee15f823
Ensure GMPParent checks whether the adapter version is present. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9cbfdbf506
Add CDMProxy::AsChromiumCDMProxy(). r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/552740d1fa23
Stub out ChromiumCDMVideoDecoder. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe49faf3ab88
Create CDM video decoder in EMEDecoderModule. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50cc93d6ec7
Add threadsafe ChromiumCDMProxy::GetCDMParent. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/832022d6eaf8
Initialize video decoder. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7c63f025a9
Implement video decoding through CDM. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06b797b7648
Implement CDM video decoder flush. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4555aebea6a
Implement CDM video decoder drain. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b8e5018f7a
Shutdown CDMVideoDecoder. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/674ee2d04472
Shutdown ChromiumCDMParent. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe4a4ae4ff80
Hook up CDM storage. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6e91518a67
Implement CDM persistent sessions. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/98c87d2f85b3
Rename DetailedPromise Status enum. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/596260b13532
Port the work around from Bug 1343140 to the new CDM video decoder architecture. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c478da01df
Ask the GMPService for the GMP thread in GMPParent::ChildTerminated. r=gerald
(In reply to Gerald Squelart [:gerald] from comment #126)
> ::: dom/media/gmp/GMPParent.cpp:387
> (Diff revision 1)
> > +  nsCOMPtr<nsIThread> gmpThread;
> > +  mps->GetThread(getter_AddRefs(gmpThread));
> > +  return gmpThread;
> The returned thread pointer is stored in a local nsCOMPtr, which is
> destroyed at the end of the function, releasing its reference.
> So in the short time ***betweeen the end of the function*** and when the
> nsIThread* is used, that nsIThread could be destroyed elsewhere, leading to
> UAF.

A small correction/clarification to the part between '***' above:
In fact, the race starts immediately after mps->GetThread has written into the local gmpThread, because if shutdown happens after that, this local gmpThread may be the only reference left, so the nsIThread object could be destroyed at the end of this function even before we return!

(The checked-in fix is still good.)
https://hg.mozilla.org/mozilla-central/rev/6e4c1af85be7
https://hg.mozilla.org/mozilla-central/rev/778ae7585a4e
https://hg.mozilla.org/mozilla-central/rev/5453681637fc
https://hg.mozilla.org/mozilla-central/rev/9baeeb80d632
https://hg.mozilla.org/mozilla-central/rev/926546bcd7aa
https://hg.mozilla.org/mozilla-central/rev/997ee5288cd0
https://hg.mozilla.org/mozilla-central/rev/f26e287c282c
https://hg.mozilla.org/mozilla-central/rev/ff38b37821b0
https://hg.mozilla.org/mozilla-central/rev/89cad106793c
https://hg.mozilla.org/mozilla-central/rev/270df6226231
https://hg.mozilla.org/mozilla-central/rev/ec3bc63b5200
https://hg.mozilla.org/mozilla-central/rev/45dd49944a96
https://hg.mozilla.org/mozilla-central/rev/ab102cdd2848
https://hg.mozilla.org/mozilla-central/rev/63434c1f9900
https://hg.mozilla.org/mozilla-central/rev/b1281784ba5b
https://hg.mozilla.org/mozilla-central/rev/e6d68905b4ec
https://hg.mozilla.org/mozilla-central/rev/46c077aafe07
https://hg.mozilla.org/mozilla-central/rev/e46942398895
https://hg.mozilla.org/mozilla-central/rev/a872ee15f823
https://hg.mozilla.org/mozilla-central/rev/8d9cbfdbf506
https://hg.mozilla.org/mozilla-central/rev/552740d1fa23
https://hg.mozilla.org/mozilla-central/rev/fe49faf3ab88
https://hg.mozilla.org/mozilla-central/rev/f50cc93d6ec7
https://hg.mozilla.org/mozilla-central/rev/832022d6eaf8
https://hg.mozilla.org/mozilla-central/rev/0f7c63f025a9
https://hg.mozilla.org/mozilla-central/rev/c06b797b7648
https://hg.mozilla.org/mozilla-central/rev/c4555aebea6a
https://hg.mozilla.org/mozilla-central/rev/47b8e5018f7a
https://hg.mozilla.org/mozilla-central/rev/674ee2d04472
https://hg.mozilla.org/mozilla-central/rev/fe4a4ae4ff80
https://hg.mozilla.org/mozilla-central/rev/2a6e91518a67
https://hg.mozilla.org/mozilla-central/rev/98c87d2f85b3
https://hg.mozilla.org/mozilla-central/rev/596260b13532
https://hg.mozilla.org/mozilla-central/rev/b5c478da01df
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: