1146796 - (GMP_for_external_rendering) GMP Hardware Rendering API

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Kilik Kuo [:kikuo] (inactive)]

For EME HW CDM case, the audio/video may directly decrypted/decoded/rendered in GMP Child process via HW decryptor/decoder/composer.

Comment 1

[James Cheng[:JamesCheng]]

I will start to work on this.

Comment 2

[James Cheng[:JamesCheng]]

Attachment: [WIP] gmp-ipdl-implementation.patch

upload my WIP.
1. GMP|Z|MediaRendererParent/Child.cpp is a temp naming. Since Bug1168053

2. Compile OK but only a prototype.

Comment 3

[James Cheng[:JamesCheng]]

Attachment: [WIP] gmp-ipdl-implementation.patch

Update the patches for Bug 1168053 and complete the prototypes.

Comment 4

[James Cheng[:JamesCheng]]

Attachment: [WIP]0001-gmp-ipdl-implementation.patch

update for some typo.

Comment 5

[James Cheng[:JamesCheng]]

Attachment: [WIP]0002-add-gmp-renderer-fake-prototype-for-gtest.patch

update for fake renderer for gtest.

Comment 6

[James Cheng[:JamesCheng]]

Attachment: [WIP]0001-gmp-ipdl-implementation.patch

rebase and update.

Comment 7

[James Cheng[:JamesCheng]]

Attachment: [WIP]0002-add-gmp-renderer-fake-prototype-for-gtest.patch

rebase and update

Comment 8

[James Cheng[:JamesCheng]]

Attachment: [WIP] add-gmp-renderer-fake-prototype-for-gtest.patch

fix build error

Comment 9

[James Cheng[:JamesCheng]]

Attachment: [WIP] gmp-ipdl-implementation.patch

Comment 10

[James Cheng[:JamesCheng]]

Attachment: [WIP] add-gmp-renderer-fake-prototype-for-gtest.patch

rebase to 60fd82095eb3c22e7bb42716849d0d48a30ee4d7 and update.

Comment 11

[James Cheng[:JamesCheng]]

Attachment: [WIP] gmp-ipdl-implementation.patch

rebase to 60fd82095eb3c22e7bb42716849d0d48a30ee4d7.

Update the implementation followed GMPVideoDecoder and GMPAudioDecoder.

Comment 12

[James Cheng[:JamesCheng]]

Attachment: [WIP] Config-for-adding-fake-gmp-renderer

rebase on a317822b091330c788e0f41562285baa4e9bde96

Add the essential configuration for the fake gmprenderer.

Comment 13

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype-for-gtest

rebase on a317822b091330c788e0f41562285baa4e9bde96

Add fake gmp child renderer. [TODO] Need to delete unused code and simulate the callback to parent process scenatio.

Comment 14

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-IPDL-implementation

rebase on a317822b091330c788e0f41562285baa4e9bde96

GMPMediaRenderer for the external renderer use case.

Comment 15

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMediaRenderer

rebase on a317822b091330c788e0f41562285baa4e9bde96

Forcely testing call GMPMediaRenderer API and get callback from GMP Child process.

It seems ok for the current patches.

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8639175 [details] [diff] [review]
[WIP] Add-gmp-renderer-fake-prototype-for-gtest

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

Hi James,

Here are my thoughts about this patch.

::: dom/media/gmp-plugin-renderer/gmp-fake.cpp
@@ +1,3 @@
> +/*!
> + * \copy
> + *     Copyright (c)  2009-2014, Cisco Systems

The copyright for Cisco is for openh264, I don't think we need this for gmp renderer.

@@ +107,5 @@
> +};
> +
> +
> +
> +class FakeDecoderTask : public GMPTask {

FakeRendererTask ?

@@ +149,5 @@
> +                                 const bool& aMissingFrames,
> +                                 const uint8_t* aCodecSpecificInfo,
> +                                 uint32_t aCodecSpecificInfoLength,
> +                                 int64_t aRenderTimeMs,
> +                                 const GMPPacketType& aPacketType)

What is GMPPacketType ? I can't find it

::: dom/media/gmp-plugin-renderer/gmp-test-decryptor.h
@@ +93,5 @@
> +private:
> +  GMPAsyncShutdownHost* mHost;
> +};
> +
> +#endif

Do we need to test this file ?

::: dom/media/gmp-plugin-renderer/gmp-test-output-protection.h
@@ +126,5 @@
> +  return;
> +}
> +
> +} // gmptest
> +} // mozilla

ditto.

::: dom/media/gmp-plugin-renderer/gmp-test-storage.h
@@ +59,5 @@
> +GMPErr
> +GMPEnumRecordNames(RecvGMPRecordIteratorPtr aRecvIteratorFunc,
> +                   void* aUserArg);
> +
> +#endif // TEST_GMP_STORAGE_H__

ditto

::: dom/media/gmp-plugin-renderer/moz.build
@@ +7,5 @@
> +# largely a copy of dom/media/gmp-fake/moz.build
> +
> +FINAL_TARGET = 'dist/bin/gmp-fakerenderer/1.0'
> +SOURCES += [
> +    'gmp-fake.cpp',

Should name it properly. Maybe 'gmp-fake-cdmrenderer' or just 'gmp-fake-renderer'.

Comment 17

[James Cheng[:JamesCheng]]

Attachment: [WIP] Config-for-adding-fake-gmp-renderer.patch

Comment 18

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype-for-gtes.patch

Comment 19

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

Comment 20

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

Comment 21

[James Cheng[:JamesCheng]]

Hi JW,

These patches are for external A/V display I just name it as GMPMediaRenderer.

I take GMPVideoDecoder and GMPAudioDecoder for reference.

And the API is designed following this document.

https://docs.google.com/presentation/d/1khdOTV8ieJqg66MygWmvf5cjq4z8LLT60-SGHOIwSBQ/edit#slide=id.g63ab247df_13


Supposedly these patches will be used by what Kilik works for external decrypt/ decode/ render.

Could you please help to give us some feedback? 

Is there anything I ignore to cover in this patch? (any test case I should create?)

Thanks.

Comment 22

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

update for other platform build error.

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8643520 [details] [diff] [review]
[WIP] Config-for-adding-fake-gmp-renderer.patch

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

Not familiar with build/test scripts. Please find the right owner.

Comment 24

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8643521 [details] [diff] [review]
[WIP] Add-gmp-renderer-fake-prototype-for-gtes.patch

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

::: dom/media/gmp-plugin-renderer/Makefile.in
@@ +9,5 @@
> +  $(SHARED_LIBRARY) \
> +  $(srcdir)/fakerenderer.info \
> +  $(srcdir)/fakerenderer.voucher
> +
> +include $(topsrcdir)/config/rules.mk

Not familiar with build files. But this is somewhat different from dom/media/gmp-plugin-openh264/Makefile.in which doesn't have $(SHARED_LIBRARY).

::: dom/media/gmp-plugin-renderer/fakerenderer.info
@@ +1,4 @@
> +Name: fakerenderer
> +Description: Fake GMP Renderer Plugin
> +Version: 1.0
> +APIs: render-media[h264:fake]

Why "h264"? The renderer should be ignorant of the encoding format.

::: dom/media/gmp-plugin-renderer/gmp-fake-renderer.cpp
@@ +26,5 @@
> +#else
> +#define PUBLIC_FUNC
> +#endif
> +
> +#define BIG_FRAME 10000

Remove the duplicate code that is also in gmp-fake-openh264.cpp and those not used at all.

@@ +57,5 @@
> +GMPPlatformAPI* g_platform_api = NULL;
> +
> +class FakeMediaRenderer;
> +
> +struct EncodedFrame {

Not used.

@@ +72,5 @@
> +class FakeRenderer;
> +class FakeRendererVideoTask : public GMPTask {
> + public:
> +  FakeRendererVideoTask(FakeMediaRenderer* renderer,
> +                        GMPVideoEncodedFrame* frame,

It should be GMPVideoi420Frame.

@@ +99,5 @@
> +};
> +
> +class FakeMediaRenderer : public GMPMediaRenderer {
> + public:
> +  explicit FakeMediaRenderer (GMPVideoHost* hostAPI) :

What is this host for?

@@ +106,5 @@
> +
> +  virtual ~FakeMediaRenderer() {
> +  }
> +
> +  virtual void InitRenderer(const GMPAudioCodec& aAudioCodecSettings,

Why do we pass audio/video codec spec?

@@ +112,5 @@
> +                            const uint8_t* aCodecSpecific,
> +                            uint32_t aVideoCodecSpecificLength,
> +                            GMPMediaRendererCallback* aCallback,
> +                            const int32_t& aCoreCount,
> +                            const GMPDisplayInfo& aDisplayInfo)

Where is GMPDisplayInfo defined?

@@ +119,5 @@
> +
> +    callback_ = aCallback;
> +  }
> +
> +  virtual void RenderVideoPacket(GMPVideoEncodedFrame* aInputFrame,

Isn't a renderer supposed to render decoded frames?

@@ +124,5 @@
> +                                 const bool& aMissingFrames,
> +                                 const uint8_t* aCodecSpecificInfo,
> +                                 uint32_t aCodecSpecificInfoLength,
> +                                 int64_t aRenderTimeMs,
> +                                 const GMPPacketType& aPacketType)

Where is GMPPacketType defined?

@@ +137,5 @@
> +    GMPLOG (GL_DEBUG, "RenderAudioPacket");
> +    g_platform_api->runonmainthread(new FakeRendererAudioTask(this, aEncodedSamples));
> +  }
> +
> +  virtual void SetPlaybackRate(const int32_t& aRate)

Why not just |int32_t aRate|? Is it something related to IPDL?

@@ +173,5 @@
> +    GMPLOG (GL_DEBUG, "\033[1;31m============ShutdownComplete============\n\033[m");
> +    callback_->ShutdownComplete(GMPErr::GMPNoErr);
> +  }
> +
> +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs)

Please document/comment each API so we know what it is for.

@@ +188,5 @@
> +  {
> +    GMPLOG (GL_DEBUG, "UpdateDisplayPosition");
> +  }
> +
> +  void SimulateRenderVideo(GMPVideoEncodedFrame* inputFrame, int64_t renderTimeMs)

How is it different from RenderVideoPacket()?

@@ +197,5 @@
> +    //callback to parent for acquiring next video frame.
> +    callback_->NotifyVideoInputNext();
> +  }
> +
> +  void SimulateRenderAudio(GMPAudioSamples* audioSample)

Ditto.

Comment 25

[James Cheng[:JamesCheng]]

(In reply to JW Wang [:jwwang] from comment #24)
> Comment on attachment 8643521 [details] [diff] [review]
> [WIP] Add-gmp-renderer-fake-prototype-for-gtes.patch
> 
> Review of attachment 8643521 [details] [diff] [review]:
Thanks for the feedback.
Hi JW, please see my response below, thank you!
> -----------------------------------------------------------------
> 
> ::: dom/media/gmp-plugin-renderer/Makefile.in
> @@ +9,5 @@
> > +  $(SHARED_LIBRARY) \
> > +  $(srcdir)/fakerenderer.info \
> > +  $(srcdir)/fakerenderer.voucher
> > +
> > +include $(topsrcdir)/config/rules.mk
> 
> Not familiar with build files. But this is somewhat different from
> dom/media/gmp-plugin-openh264/Makefile.in which doesn't have
> $(SHARED_LIBRARY).
> 
I'm also not familiar with the build file. I just modified from 

https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp-plugin/Makefile.in

but I think open264 case is much more suitable for our case. so I will modified it into open264-like.

https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp-plugin-openh264/Makefile.in
> ::: dom/media/gmp-plugin-renderer/fakerenderer.info
> @@ +1,4 @@
> > +Name: fakerenderer
> > +Description: Fake GMP Renderer Plugin
> > +Version: 1.0
> > +APIs: render-media[h264:fake]
> 
> Why "h264"? The renderer should be ignorant of the encoding format.
> 
Thanks, I will modified it into |APIs: render-media[external.renderer:fake]|
And also modified the GMPMediaRenderer.cpp(in other patch) into 
GMPMediaRenderer::InitTags(nsTArray<nsCString>& aTags)
{
  aTags.AppendElement(NS_LITERAL_CSTRING("external.renderer"));

since the tag is necessary for comparing if it is the target component we want to load.
> ::: dom/media/gmp-plugin-renderer/gmp-fake-renderer.cpp
> @@ +26,5 @@
> > +#else
> > +#define PUBLIC_FUNC
> > +#endif
> > +
> > +#define BIG_FRAME 10000
> 
> Remove the duplicate code that is also in gmp-fake-openh264.cpp and those
> not used at all.
> 
Addressed
> @@ +57,5 @@
> > +GMPPlatformAPI* g_platform_api = NULL;
> > +
> > +class FakeMediaRenderer;
> > +
> > +struct EncodedFrame {
> 
> Not used.
> 
Deleted
> @@ +72,5 @@
> > +class FakeRenderer;
> > +class FakeRendererVideoTask : public GMPTask {
> > + public:
> > +  FakeRendererVideoTask(FakeMediaRenderer* renderer,
> > +                        GMPVideoEncodedFrame* frame,
> 
> It should be GMPVideoi420Frame.
> 
Our design is to decrypt or decode and render outside. So I think we will bypass encoded video data from the future work of Kilik's patch.
> @@ +99,5 @@
> > +};
> > +
> > +class FakeMediaRenderer : public GMPMediaRenderer {
> > + public:
> > +  explicit FakeMediaRenderer (GMPVideoHost* hostAPI) :
> 
> What is this host for?
> 
The host is the API parameter which is the library exposed interface.
https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp?offset=700#405

The caller is similar to https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPContentChild.cpp#84
 
> @@ +106,5 @@
> > +
> > +  virtual ~FakeMediaRenderer() {
> > +  }
> > +
> > +  virtual void InitRenderer(const GMPAudioCodec& aAudioCodecSettings,
> 
> Why do we pass audio/video codec spec?
> 
Since we assume and expect the renderer needs to decode / decrypt, we need to pass the information just like GMPVideoDecoder and GMPAudioDecoder does...


> @@ +112,5 @@
> > +                            const uint8_t* aCodecSpecific,
> > +                            uint32_t aVideoCodecSpecificLength,
> > +                            GMPMediaRendererCallback* aCallback,
> > +                            const int32_t& aCoreCount,
> > +                            const GMPDisplayInfo& aDisplayInfo)
> 
> Where is GMPDisplayInfo defined?
> 
I just define it in the next patch for IPDL implementation.
Please refer to this,
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1146796&attachment=8643564

> @@ +119,5 @@
> > +
> > +    callback_ = aCallback;
> > +  }
> > +
> > +  virtual void RenderVideoPacket(GMPVideoEncodedFrame* aInputFrame,
> 
> Isn't a renderer supposed to render decoded frames?
> 
We think it designed for encoded frames.
> @@ +124,5 @@
> > +                                 const bool& aMissingFrames,
> > +                                 const uint8_t* aCodecSpecificInfo,
> > +                                 uint32_t aCodecSpecificInfoLength,
> > +                                 int64_t aRenderTimeMs,
> > +                                 const GMPPacketType& aPacketType)
> 
> Where is GMPPacketType defined?
> 
Sorry~~
Also refer to 
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1146796&attachment=8643564


> @@ +137,5 @@
> > +    GMPLOG (GL_DEBUG, "RenderAudioPacket");
> > +    g_platform_api->runonmainthread(new FakeRendererAudioTask(this, aEncodedSamples));
> > +  }
> > +
> > +  virtual void SetPlaybackRate(const int32_t& aRate)
> 
> Why not just |int32_t aRate|? Is it something related to IPDL?
> 
Oh, the IPDL code generator just generate the prototype like
virtual bool RecvSetPlaybackRate(const int32_t& aRate) override;
So I just pass the parameter with const reference to avoid copy.
> @@ +173,5 @@
> > +    GMPLOG (GL_DEBUG, "\033[1;31m============ShutdownComplete============\n\033[m");
> > +    callback_->ShutdownComplete(GMPErr::GMPNoErr);
> > +  }
> > +
> > +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs)
> 
> Please document/comment each API so we know what it is for.
> 
OK, I will add comment in IPDL file for documentation.
> @@ +188,5 @@
> > +  {
> > +    GMPLOG (GL_DEBUG, "UpdateDisplayPosition");
> > +  }
> > +
> > +  void SimulateRenderVideo(GMPVideoEncodedFrame* inputFrame, int64_t renderTimeMs)
> 
> How is it different from RenderVideoPacket()?
> 
> @@ +197,5 @@
> > +    //callback to parent for acquiring next video frame.
> > +    callback_->NotifyVideoInputNext();
> > +  }
> > +
> > +  void SimulateRenderAudio(GMPAudioSamples* audioSample)
> 
> Ditto.
The calling flow is RenderVideoPacket-> Create a Task -> Task::Run-> Callback to this SimulateRenderVideo

Since it is a fake renderer, so we just do nothing but invoking callback->NotifyNext... to parent side.

Does it make sense to you?

Thank you for your feedback.

Comment 26

[JW Wang [:jwwang] [:jw_wang]]

No, it doesn't make sense to me. Decryption is the job of a CDM and decoding is the job of a decoder. A renderer should only focus on presenting decoded content.

Comment 27

[James Cheng[:JamesCheng]]

Comment on attachment 8643520 [details] [diff] [review]
[WIP] Config-for-adding-fake-gmp-renderer.patch

Hi Chris,

I'm James co-working with Kilik for the backend GMP for external rendering.

I want to inject a fake GMP media renderer module in gecko.

And I just follow the |gmp-plugin| module to add some configurations in the corresponding files.

Could you please help me to take a look of this patch? 

Please point me that if anything I ignored to add in other files.


We will keep refining and revising the other patches with JW Wang and Kilik.

Thank you very much.

Comment 28

[James Cheng[:JamesCheng]]

Hi JW,

For HW CDM Rendering in EME case,

We think the encoded and encrypted data will sink through GMP rendering API and do the decode and decrypt in the CDM process itself.

We don't want to seperate the API into decrypt, decode and render independently. 

Maybe we can discuss for the naming of the APIs in order to emphasize that the API will also do decrypt or decode.

Let's have a face two face discuss to sync up our design strategy.

Thanks for pointing out the confusion.

Comment 29

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

Comment on attachment 8643520 [details] [diff] [review]
[WIP] Config-for-adding-fake-gmp-renderer.patch

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

Edwin did the work to make gmp-clearkey run, so he'd be better to f? this.

Also, IIRC :ted reviewed edwin's patches, so he'd probably be reviewing this. Edwin can confirm that.

Comment 30

[James Cheng[:JamesCheng]]

(In reply to Chris Pearce (:cpearce) from comment #29)
> Comment on attachment 8643520 [details] [diff] [review]
> [WIP] Config-for-adding-fake-gmp-renderer.patch
> 
> Review of attachment 8643520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Edwin did the work to make gmp-clearkey run, so he'd be better to f? this.
> 
> Also, IIRC :ted reviewed edwin's patches, so he'd probably be reviewing
> this. Edwin can confirm that.

Thank you Chris.

Hi Edwin, 
Please help us for the configuration of the gmp fake renderer.

Thanks.

Comment 31

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype.patch

Hi Chris,

As Comment 24 from JW,

This patch is a fake gmp renderer simply took gmp-plugin-openh264 as reference.

We want to add a fake gmp renderer for testing the IPDL implementation.

Could you please give us some feedback or suggestion if there is anything I ignore or not correct? 
I'm not sure if I need to create a gtest for testing this? (since I'm not familiar with gtest, is there any recommended sample on DXR we can take reference?)

Thanks you very much.

Comment 32

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

Rebase for current code build error.

And add comment on ipdl file.

Comment 33

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

Rebase for current code base build error.

Comment 34

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

Comment on attachment 8648601 [details] [diff] [review]
[WIP] GMP-ipdl-implementation.patch

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

You've made a lot of impressive progress here, but I need to make some comments on the GMP API header. We need to keep it as consistent as possible with the rest of the media stack and GMP/EME code, and some of the APIs need tweaking to work better with Gecko's media code.

::: dom/media/gmp/gmp-api/gmp-media-render.h
@@ +45,5 @@
> +
> +enum GMPMediaPlayState
> +{
> +  GMP_Pause = 0,
> +  GMP_Play,

Please set all the values in the enum, i.e.:

enum GMPMediaPlayState {
  kGMPPause = 0,
  kGMPPlay = 1,
  kGMPMediaPlayStateInvalid = 2
};

@@ +46,5 @@
> +enum GMPMediaPlayState
> +{
> +  GMP_Pause = 0,
> +  GMP_Play,
> +  GMP_Seek,

Why do you need a GMP_Seek state?

Also, please use the kGMPEnumValue stype that most of the GMPAPI uses for enums (as I used in my preceding comment).

Please match the prevailing style of the code you're working on, otherwise the code devolves into an inconsistent mess.

@@ +64,5 @@
> +  int32_t mTop;
> +  int32_t mLeft;
> +  int32_t mWidth;
> +  int32_t mHeight;
> +};

Please stop passing primitives (ints, bools, doubles, etc) as arguments by const reference.

Also, please don't primitive make parameters const, it's most of the other GMP APIs don't do that, and I don't want to increase the level of inconsistency in the API.

You need comments on these methods to explain what the functions do and how they're supposed to be called.

@@ +66,5 @@
> +  int32_t mWidth;
> +  int32_t mHeight;
> +};
> +
> +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD

You actually don't need to make these methods main thread only if the implementation proxies the implementation to the main thread if they're not called on the main thread. See GMPDecryptorChild.cpp for an example. You'll need to ensure data being proxied to the main thread are copied in a threadsafe way (like strings for example).

@@ +72,5 @@
> +{
> +public:
> +  virtual ~GMPMediaRendererCallback() {}
> +
> +  virtual void NotifyElapsedTime(const int64_t aElapsedTimeMs) = 0;

ms means "milliseconds".

Milliseconds are not precise enough for video playback time stamps. You will get rounding errors. You should use microseconds.

Also, please call this TimeUpdate(), to match the "timeupdate" event in the HTMLMediaElement.

@@ +74,5 @@
> +  virtual ~GMPMediaRendererCallback() {}
> +
> +  virtual void NotifyElapsedTime(const int64_t aElapsedTimeMs) = 0;
> +
> +  virtual void NotifyAudioRenderEOS() = 0;

Drop the "Notify" prefix on these methods; the precedent set in other GMP APIs is to have callback interface function names as past-tense verbs.

So this would be something like "AudioRenderReachedEOS()".

@@ +78,5 @@
> +  virtual void NotifyAudioRenderEOS() = 0;
> +
> +  virtual void NotifyVideoRenderEOS() = 0;
> +
> +  virtual void NotifyAudioInputNext() = 0;

What does this function to? Request more input? If so, please call it AudioInputDataExhausted() to match the GMP{Audio,Video}DecoderCallback interfaces.

@@ +82,5 @@
> +  virtual void NotifyAudioInputNext() = 0;
> +
> +  virtual void NotifyVideoInputNext() = 0;
> +
> +  virtual void ShutdownComplete(const GMPErr aErr) = 0;

I don't think it makes sense to report an error on shutdown. What are you going to do if shutdown throws an error? Shutdown? Also, if a GMP wanted to report an error during shutdown, it could call the Error() callback.

@@ +88,5 @@
> +  virtual void SetOverlayImageID(const uint32_t aId) = 0;
> +
> +  // Called when the decoder encounters a catestrophic error and cannot
> +  // continue. Gecko will not send any more input for decoding.
> +  virtual void Error(const GMPErr aError) = 0;

We've learned while trying to ship EME that passing more than just an integer error code is needed for debugging in the field. Sites using EME really need more information than a single integer value. So please add to this Error() function a const char* message parameter, and a uint32_t length, i.e.:

virtual void Error(const GMPErr aError, const char* aMessage, uint32_t aLength) = 0;

We can percolate that up to JS, or log it to the browser console at least, to assist in debugging.

@@ +94,5 @@
> +
> +#define GMP_API_MEDIA_RENDERER "render-media"
> +
> +//[TODO]
> +// Media Rendering for a single stream. A GMP may be asked to create multiple

Gecko currently only instantiates one video decoder per <video> at once, including for MSE video. During playback however, we do shutdown and re-create video decoders when the resolution of the video stream changes, or if we make the video element dormant.

If there are multiple videos requiring playback at once, you'd want multiple GMPMediaRenderer instances instantiated by the GMP.

@@ +106,5 @@
> +{
> +public:
> +  virtual ~GMPMediaRenderer() {}
> +
> +  virtual void InitRenderer(const GMPAudioCodec& aAudioCodecSettings,

You need to separate InitRenderer() into distinct InitVideoDecode() and InitAudioDecode() functions, because often the decoders need to be reinitialized seamlessly during playback video decoder. For example, if we're doing MSE+EME playback, and we switch video streams to a higher resolution stream, we need to re-init the video decoder.

So I think the easiest way to do this is for you to document in comments that InitVideoDecode() and InitAudioDecode() can be called multiple times, and when they are re-called the implementation should drain any pending samples out of the decoder, shutdown the decoder, and then re-init the decoder with the new parameters. Input fed after the Init{Audio,Video}Decode() call should be decoded with the new decoder configuration.

Currently all the players we support don't re-init the audio decoder at runtime, but our API design should allow that so that in future we can if we need to.

@@ +111,5 @@
> +                            const GMPVideoCodec& aVideoCodecSettings,
> +                            const uint8_t* aVideoCodecSpecific,
> +                            uint32_t aVideoCodecSpecificLength,
> +                            GMPMediaRendererCallback* aCallback,
> +                            const int32_t& aCoreCount,

Please pass primitives (like int32_t aCoreCount) by value rather than by reference.

It's also sloppy to to be inconsistent and mix styles; aVideoCodecSpecificLength is not passed by const reference but aCoreCount is for example.

@@ +118,5 @@
> +  virtual void RenderVideoPacket(GMPVideoEncodedFrame* aInputFrame,
> +                                 const bool& aMissingFrames,
> +                                 const uint8_t* aCodecSpecificInfo,
> +                                 uint32_t aCodecSpecificInfoLength,
> +                                 int64_t aRenderTimeMs,

The GMPVideoEncodedFrame has a Timestamp() method, why do you need to pass a render time?

@@ +124,5 @@
> +
> +  virtual void RenderAudioPacket(GMPAudioSamples* aEncodedSamples,
> +                                 const GMPPacketType& aPacketType) = 0;
> +
> +  virtual void SetPlaybackRate(const int32_t& aRate) = 0;

Please pass primitives by value rather than by reference.

Use a double for the rate. The rest of our playback rate code uses doubles for rates. We also want to be able to represent non-whole number rates.

@@ +126,5 @@
> +                                 const GMPPacketType& aPacketType) = 0;
> +
> +  virtual void SetPlaybackRate(const int32_t& aRate) = 0;
> +
> +  virtual void SetVolume(const double& aVolume) = 0;

Please pass primitives by value rather than by reference.

@@ +128,5 @@
> +  virtual void SetPlaybackRate(const int32_t& aRate) = 0;
> +
> +  virtual void SetVolume(const double& aVolume) = 0;
> +
> +  virtual void ShutDown() = 0;

You need to document here that ShutdownComplete() needs to be called.

@@ +130,5 @@
> +  virtual void SetVolume(const double& aVolume) = 0;
> +
> +  virtual void ShutDown() = 0;
> +
> +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs) = 0;

So... I'm guessing (because you don't have any comments) that FlushAndDropUntil($time) is called when you seek to $time and then feed data into the renderer from the preceding video and audio sync points up to the seek target? How do you know when to stop feeding input data after the flush? You need a FlushUntilComplete() callback.

I'm still not sure if you need the explicit Seek state. The renderer doesn't play while seeking right, so it's the same as paused?

Can't you just go:

Host calls SetState(Pause) (if decoder was playing at the time the seek came in)
Host calls FlushUntil($seekTarget)
Until GMP calls FlushUntilComplete(), whenever GMP calls Input{Audio,Video}Exhausted(), the Host calls Render{Audio,Video}Packet. This means the GMP tells us when it's finished seeking the decoder.
Host calls SetState(Play) (if decoder was playing at the time the seek came in)

Also, what about canceling a seek if a new seek comes in?

I think you're better off calling this method SeekTo($time), rather than FlushAndDropUntil($time), since seeking is really what's going on.

And microseconds timestamps.

@@ +134,5 @@
> +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs) = 0;
> +
> +  virtual void StateChange(const GMPMediaPlayState& aState) = 0;
> +
> +  virtual void UpdateDisplayPosition(const GMPDisplayInfo& aDisplayInfo) = 0;

SetDisplayPosition() ?

You didn't name SetVolume() "UpdateVolume()", did you?

Comment 35

[JW Wang [:jwwang] [:jw_wang]]

How will MediaDataRenderer{Proxy} play with the existing AudioSink and the incoming VideoSink which kilik is working on? AudioSink is a renderer as well as the VideoSink which share media queues with a content producer which is MDSM. I want to reuse this pattern instead of inventing a new paradigm.

It seems to against our goal to move A/V sync out of MDSM because gecko has to call Render{Video,Audio}Packet manually. I would expect a queue is shared between MDSM and the renderer which will consume the data in the queue without much interaction from MDSM.

So the APIs will be much similiar to those of AudioSink where you have:
1. Pause to stop rendering
2. Resume to start rendering again
3. Shutdown to tear down the renderer

Seeking can be handled as follows:
1. MDSM tells the renderer to stop rendering
2. MDSM clear the queue
3. MDSM tell the renderer to start rendering again whatever is in the queue.

This is exactly how MDSM interacts with AudioSink to handle seek.

In my opinion, a renderer should as simple as rendering whatever content at hand.

Comment 36

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #34)
> Comment on attachment 8648601 [details] [diff] [review]
> [WIP] GMP-ipdl-implementation.patch 
> @@ +130,5 @@
> > +  virtual void SetVolume(const double& aVolume) = 0;
> > +
> > +  virtual void ShutDown() = 0;
> > +
> > +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs) = 0;
> 
> So... I'm guessing (because you don't have any comments) that
> FlushAndDropUntil($time) is called when you seek to $time and then feed data
> into the renderer from the preceding video and audio sync points up to the
> seek target? How do you know when to stop feeding input data after the
> flush? You need a FlushUntilComplete() callback.
> 
> I'm still not sure if you need the explicit Seek state. The renderer doesn't
> play while seeking right, so it's the same as paused?
> 
> Can't you just go:
> 
> Host calls SetState(Pause) (if decoder was playing at the time the seek came
> in)
> Host calls FlushUntil($seekTarget)
> Until GMP calls FlushUntilComplete(), whenever GMP calls
> Input{Audio,Video}Exhausted(), the Host calls Render{Audio,Video}Packet.
> This means the GMP tells us when it's finished seeking the decoder.
> Host calls SetState(Play) (if decoder was playing at the time the seek came
> in)
> 
> Also, what about canceling a seek if a new seek comes in?
> 
> I think you're better off calling this method SeekTo($time), rather than
> FlushAndDropUntil($time), since seeking is really what's going on.
> 

Chris, thanks for your feedback :)

I was think that it would be good to add APIs as few as possible, so considering FlushAndDropUnitl($time), my intention was to treat Host as a commander notifying GMP that Seek begins.  Both Seek($time) in Host and FlushAndDropUntil($time) in GMP are async independently. Once seek's done in Host, demuxed media data is gonna be fed into {Raw,Encrypted}AudioDataSink and waiting for GMP's 1st Input{Audio,Video}Exhausted() call after Host called FlushAndDropUntil($time). And I didn't consider of the following situation,

Host starts seeking
Host calls FlushAndDropUntil($time),
Host calls Render{Audio,Video}Data until GMP calls the 1st Input{Audio,Video}Exhausted() and media data is fed into {Raw,Encrypted}AudioDataSink.

But the 1st Input{Audio,Video}Exhausted() call may be already triggered from GMP before GMP receives FlushAndDropUntil($time).

So yes, a FlushAndDropUntilCompleted() could resolve this situation !!  Cool ~


Regarding calling the method FlushAndDropUntil() or SeekTo(), 
my original design was called SeekToPosition() and after having a discussion with you on March (wow...quite long time ago), I decided to split it into 2 separate parts, one is FlushAndDropUntil($time) which does everything in GMP exactly like the method name; another is adding Seek State to notify GMP state change.
Though it seems not necessary to have a Seek state now.

Comment 37

[James Cheng[:JamesCheng]]

Hi Chris, 

Thanks for your feedback.
According to your feedback above, just want to be clear, 
Could you please tell me the basic coding convention in GMP related code for |const| and |const &|?

IPDL code generator will add the |const &| such as 
https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPVideoDecoderParent.h?offset=600#70
https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPVideoDecoderParent.h?offset=600#75
in function prototype....

I'm not sure if I need to remove all the const& modifiers or not. 

And for |const| case, I think I can remove all the modifiers in primitive types.

Please give me some guidelines for my confusion.

Thank you very much!

Comment 38

[JW Wang [:jwwang] [:jw_wang]]

You can't change the signature when overriding a function. Otherwise it becomes overloading. For other general cases, use pass-by-value when it comes to primitives or simple types where copy is more efficient than deref.

Comment 39

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

What JW said. You can't change the types of generated IPDL interfaces that you override. But you should pass primitives by value.

Comment 40

[James Cheng[:JamesCheng]]

Hi Chris and JW,
In addition,

https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPVideoDecoderChild.h?offset=0#36

(Passing parameter to IPDL-related API)
It just remains the |const| in the prototype.
For this case, |const| needs to be removed as you recommended?

As the result, 
I should not use |const| anymore for non-IPDL-related code even they will pass to the |const&| IPDL API.
Is that right?
Thank you.

Comment 41

[JW Wang [:jwwang] [:jw_wang]]

It doesn't really matter to say
void Foo(int i); or void Foo(const int i);

I would prefer |int i| for less typing and the callee can modify if it wants to which is however generally a bad practice to modify the parameters which are not for return values.

Comment 42

[James Cheng[:JamesCheng]]

(In reply to [PTO] Chris Pearce (:cpearce) from comment #34)
> Comment on attachment 8648601 [details] [diff] [review]
> [WIP] GMP-ipdl-implementation.patch
> 
> Review of attachment 8648601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
Thanks for your valuable feedbacks,
I will update the patches as your feedback.
Please see my comment below,
> You've made a lot of impressive progress here, but I need to make some
> comments on the GMP API header. We need to keep it as consistent as possible
> with the rest of the media stack and GMP/EME code, and some of the APIs need
> tweaking to work better with Gecko's media code.
> 
> ::: dom/media/gmp/gmp-api/gmp-media-render.h
> @@ +45,5 @@
> > +
> > +enum GMPMediaPlayState
> > +{
> > +  GMP_Pause = 0,
> > +  GMP_Play,
> 
> Please set all the values in the enum, i.e.:
> 
Addressed.
> enum GMPMediaPlayState {
>   kGMPPause = 0,
>   kGMPPlay = 1,
>   kGMPMediaPlayStateInvalid = 2
> };
> 
> @@ +46,5 @@
> > +enum GMPMediaPlayState
> > +{
> > +  GMP_Pause = 0,
> > +  GMP_Play,
> > +  GMP_Seek,
> 
> Why do you need a GMP_Seek state?
> 
It seems not necessary,  thanks for pointing out.

> Also, please use the kGMPEnumValue stype that most of the GMPAPI uses for
> enums (as I used in my preceding comment).
> 
> Please match the prevailing style of the code you're working on, otherwise
> the code devolves into an inconsistent mess.
>
Addressed. 
> @@ +64,5 @@
> > +  int32_t mTop;
> > +  int32_t mLeft;
> > +  int32_t mWidth;
> > +  int32_t mHeight;
> > +};
> 
> Please stop passing primitives (ints, bools, doubles, etc) as arguments by
> const reference.
> 
Addressed, I removed |const ref| into |non-const value type|

> Also, please don't primitive make parameters const, it's most of the other
> GMP APIs don't do that, and I don't want to increase the level of
> inconsistency in the API.
> 
> You need comments on these methods to explain what the functions do and how
> they're supposed to be called.
> 
> @@ +66,5 @@
> > +  int32_t mWidth;
> > +  int32_t mHeight;
> > +};
> > +
> > +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD
> 
> You actually don't need to make these methods main thread only if the
> implementation proxies the implementation to the main thread if they're not
> called on the main thread. See GMPDecryptorChild.cpp for an example. You'll
> need to ensure data being proxied to the main thread are copied in a
> threadsafe way (like strings for example).
> 
> @@ +72,5 @@
> > +{
> > +public:
> > +  virtual ~GMPMediaRendererCallback() {}
> > +
> > +  virtual void NotifyElapsedTime(const int64_t aElapsedTimeMs) = 0;
> 
> ms means "milliseconds".
> 
I've removed the |Ms|, thanks.

> Milliseconds are not precise enough for video playback time stamps. You will
> get rounding errors. You should use microseconds.
> 
> Also, please call this TimeUpdate(), to match the "timeupdate" event in the
> HTMLMediaElement.

Addressed.

> 
> @@ +74,5 @@
> > +  virtual ~GMPMediaRendererCallback() {}
> > +
> > +  virtual void NotifyElapsedTime(const int64_t aElapsedTimeMs) = 0;
> > +
> > +  virtual void NotifyAudioRenderEOS() = 0;
> 
> Drop the "Notify" prefix on these methods; the precedent set in other GMP
> APIs is to have callback interface function names as past-tense verbs.
> 
> So this would be something like "AudioRenderReachedEOS()".
> 
Addressed, thanks.

> @@ +78,5 @@
> > +  virtual void NotifyAudioRenderEOS() = 0;
> > +
> > +  virtual void NotifyVideoRenderEOS() = 0;
> > +
> > +  virtual void NotifyAudioInputNext() = 0;
> 
> What does this function to? Request more input? If so, please call it
> AudioInputDataExhausted() to match the GMP{Audio,Video}DecoderCallback
> interfaces.
> 
Addressed.

> @@ +82,5 @@
> > +  virtual void NotifyAudioInputNext() = 0;
> > +
> > +  virtual void NotifyVideoInputNext() = 0;
> > +
> > +  virtual void ShutdownComplete(const GMPErr aErr) = 0;
> 
> I don't think it makes sense to report an error on shutdown. What are you
> going to do if shutdown throws an error? Shutdown? Also, if a GMP wanted to
> report an error during shutdown, it could call the Error() callback.
> 
Addressed.

> @@ +88,5 @@
> > +  virtual void SetOverlayImageID(const uint32_t aId) = 0;
> > +
> > +  // Called when the decoder encounters a catestrophic error and cannot
> > +  // continue. Gecko will not send any more input for decoding.
> > +  virtual void Error(const GMPErr aError) = 0;
> 
> We've learned while trying to ship EME that passing more than just an
> integer error code is needed for debugging in the field. Sites using EME
> really need more information than a single integer value. So please add to
> this Error() function a const char* message parameter, and a uint32_t
> length, i.e.:
> 
> virtual void Error(const GMPErr aError, const char* aMessage, uint32_t
> aLength) = 0;
> 
> We can percolate that up to JS, or log it to the browser console at least,
> to assist in debugging.
> 
I'm not sure if I need to modify the IPDL prototype. Currently, I modify the non-IPDL functions.
And log the error message in 
void
GMPMediaRendererChild::Error(GMPErr aError, const char* aMessage, uint32_t aLength)
{
  MOZ_ASSERT(mPlugin->GMPMessageLoop() == MessageLoop::current());
  LOGD(("%s::%s: %p GMPErr = %d ErrorMessage = %s", __CLASS__, __FUNCTION__, this, aError, aMessage));
  unused << SendError(aErr);
}

Is |aLength| for non-null terminate string in case?

> @@ +94,5 @@
> > +
> > +#define GMP_API_MEDIA_RENDERER "render-media"
> > +
> > +//[TODO]
> > +// Media Rendering for a single stream. A GMP may be asked to create multiple
> 
> Gecko currently only instantiates one video decoder per <video> at once,
> including for MSE video. During playback however, we do shutdown and
> re-create video decoders when the resolution of the video stream changes, or
> if we make the video element dormant.
> 
> If there are multiple videos requiring playback at once, you'd want multiple
> GMPMediaRenderer instances instantiated by the GMP.
> 
> @@ +106,5 @@
> > +{
> > +public:
> > +  virtual ~GMPMediaRenderer() {}
> > +
> > +  virtual void InitRenderer(const GMPAudioCodec& aAudioCodecSettings,
> 
> You need to separate InitRenderer() into distinct InitVideoDecode() and
> InitAudioDecode() functions, because often the decoders need to be
> reinitialized seamlessly during playback video decoder. For example, if
> we're doing MSE+EME playback, and we switch video streams to a higher
> resolution stream, we need to re-init the video decoder.
> 
> So I think the easiest way to do this is for you to document in comments
> that InitVideoDecode() and InitAudioDecode() can be called multiple times,
> and when they are re-called the implementation should drain any pending
> samples out of the decoder, shutdown the decoder, and then re-init the
> decoder with the new parameters. Input fed after the
> Init{Audio,Video}Decode() call should be decoded with the new decoder
> configuration.
> 
> Currently all the players we support don't re-init the audio decoder at
> runtime, but our API design should allow that so that in future we can if we
> need to.
> 
Thanks, I've separated into |InitVideoRenderer/InitAudioRenderer| for the whole patch. 

> @@ +111,5 @@
> > +                            const GMPVideoCodec& aVideoCodecSettings,
> > +                            const uint8_t* aVideoCodecSpecific,
> > +                            uint32_t aVideoCodecSpecificLength,
> > +                            GMPMediaRendererCallback* aCallback,
> > +                            const int32_t& aCoreCount,
> 
> Please pass primitives (like int32_t aCoreCount) by value rather than by
> reference.
> 
> It's also sloppy to to be inconsistent and mix styles;
> aVideoCodecSpecificLength is not passed by const reference but aCoreCount is
> for example.
> 
Addressed.

> @@ +118,5 @@
> > +  virtual void RenderVideoPacket(GMPVideoEncodedFrame* aInputFrame,
> > +                                 const bool& aMissingFrames,
> > +                                 const uint8_t* aCodecSpecificInfo,
> > +                                 uint32_t aCodecSpecificInfoLength,
> > +                                 int64_t aRenderTimeMs,
> 
> The GMPVideoEncodedFrame has a Timestamp() method, why do you need to pass a
> render time?
> 
Thanks for reminding, I removed this parameter.

> @@ +124,5 @@
> > +
> > +  virtual void RenderAudioPacket(GMPAudioSamples* aEncodedSamples,
> > +                                 const GMPPacketType& aPacketType) = 0;
> > +
> > +  virtual void SetPlaybackRate(const int32_t& aRate) = 0;
> 
> Please pass primitives by value rather than by reference.
Addressed.
> 
> Use a double for the rate. The rest of our playback rate code uses doubles
> for rates. We also want to be able to represent non-whole number rates.
> 
Addressed.
> @@ +126,5 @@
> > +                                 const GMPPacketType& aPacketType) = 0;
> > +
> > +  virtual void SetPlaybackRate(const int32_t& aRate) = 0;
> > +
> > +  virtual void SetVolume(const double& aVolume) = 0;
> 
> Please pass primitives by value rather than by reference.
> 
Addressed.
> @@ +128,5 @@
> > +  virtual void SetPlaybackRate(const int32_t& aRate) = 0;
> > +
> > +  virtual void SetVolume(const double& aVolume) = 0;
> > +
> > +  virtual void ShutDown() = 0;
> 
> You need to document here that ShutdownComplete() needs to be called.
> 
Addressed.
> @@ +130,5 @@
> > +  virtual void SetVolume(const double& aVolume) = 0;
> > +
> > +  virtual void ShutDown() = 0;
> > +
> > +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs) = 0;
> 
> So... I'm guessing (because you don't have any comments) that
> FlushAndDropUntil($time) is called when you seek to $time and then feed data
> into the renderer from the preceding video and audio sync points up to the
> seek target? How do you know when to stop feeding input data after the
> flush? You need a FlushUntilComplete() callback.
> 
> I'm still not sure if you need the explicit Seek state. The renderer doesn't
> play while seeking right, so it's the same as paused?
> 
> Can't you just go:
> 
> Host calls SetState(Pause) (if decoder was playing at the time the seek came
> in)
> Host calls FlushUntil($seekTarget)
> Until GMP calls FlushUntilComplete(), whenever GMP calls
> Input{Audio,Video}Exhausted(), the Host calls Render{Audio,Video}Packet.
> This means the GMP tells us when it's finished seeking the decoder.
> Host calls SetState(Play) (if decoder was playing at the time the seek came
> in)
> 
> Also, what about canceling a seek if a new seek comes in?
> 
> I think you're better off calling this method SeekTo($time), rather than
> FlushAndDropUntil($time), since seeking is really what's going on.
> 
> And microseconds timestamps.
> 
Thanks for clarifying. I have added |SeekToComplete| API for callback needed.
> @@ +134,5 @@
> > +  virtual void FlushAndDropUntil(const int64_t& aFlushAndDropUntilTimeMs) = 0;
> > +
> > +  virtual void StateChange(const GMPMediaPlayState& aState) = 0;
> > +
> > +  virtual void UpdateDisplayPosition(const GMPDisplayInfo& aDisplayInfo) = 0;
> 
> SetDisplayPosition() ?
> 
> You didn't name SetVolume() "UpdateVolume()", did you?

Addressed, Thank you.

I updated the patches, please feedback me for further details.

Thank you very much.

Comment 43

[James Cheng[:JamesCheng]]

Attachment: [WIP] config-for-adding-fake-gmp-renderer.patch

Hi Edwin, 
Please help to feedback this configuration patch for injecting a fake GMP.

Thanks.

Comment 44

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype.patch

Rebase and update for Chris' feedback.

Comment 45

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

Update as Chris' comment.

Comment 46

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

Update as Chris' feedback.
This is just a testing code for proving if I can call GMP parent API to GMP child without any error.

Comment 47

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8651605 [details] [diff] [review]
[WIP] Testing-code-for-initialize-the-GMPMedia.patch

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

You didn't answer my questions in comment 35. The test code doesn't make much sense to be inserted into MediaFormatReader.cpp. I would like to see how the renderer fits into the existing system so we can reason about the design of its APIs.

Comment 48

[James Cheng[:JamesCheng]]

(In reply to JW Wang [:jwwang] from comment #47)
> Comment on attachment 8651605 [details] [diff] [review]
> [WIP] Testing-code-for-initialize-the-GMPMedia.patch
> 
> Review of attachment 8651605 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You didn't answer my questions in comment 35. The test code doesn't make
> much sense to be inserted into MediaFormatReader.cpp. I would like to see
> how the renderer fits into the existing system so we can reason about the
> design of its APIs.

Hi JW,
Sorry for that.
Currently, I am not quite familiar with the trend of future API refactorying.
I am keeping discussing with Kilik in order to fulfill the future Audio and Video Sink API.

Thanks.

Comment 49

[James Cheng[:JamesCheng]]

Attachment: GMPMediaRenderer Class Diagram

Upload a class diagram for quickly distinguish among the similar classes' name.

Comment 50

[James Cheng[:JamesCheng]]

Hi Edwin,
Thanks for your feedback.
Please let me know who might be the reviewer for this patch.
Thank you.

Comment 51

[James Cheng[:JamesCheng]]

Hi JW,

After discussing with Kilik,

The Rendering APIs is internally used in the future encrypted Audio/Video Sink.

MDSM is interacting with A/V Sink, and A/V Sink is interacting with GMP Rendering API.

Currently, we don't think there is any incompatibility for the future A/V Sink design.

How do you think of?

Thank you very much.

Comment 52

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to James Cheng[:JamesCheng] from comment #50)
> Hi Edwin,
> Thanks for your feedback.
> Please let me know who might be the reviewer for this patch.
> Thank you.

I usually ask :glandium for build system reviews.

Comment 53

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8651604 [details] [diff] [review]
[WIP] GMP-ipdl-implementation.patch

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

::: dom/media/gmp/PGMPMediaRenderer.ipdl
@@ +29,5 @@
> +                          GMPDisplayInfo aDisplayInfo);
> +  //Init Renderer for audio, passing the audio spec to child.
> +  async InitAudioRenderer(GMPAudioCodecData aAudioCodecSettings);
> +  //Bypass the encoded/encrypt video frame to child for rendering.
> +  async RenderVideoPacket(GMPVideoEncodedFrameData aInputFrame,

You might want to send multiple packets at a time to reduce IPC overhead.

@@ +37,5 @@
> +  //Bypass the encoded/encrypt audio sample to child.
> +  async RenderAudioPacket(GMPAudioEncodedSampleData aInput,
> +                          GMPPacketType aPacketType);
> +  async SetPlaybackRate(double aRate);
> +  async SetVolume(double aVolume);

Don't you need something like AudioSink::SetPreservesPitch()?

@@ +39,5 @@
> +                          GMPPacketType aPacketType);
> +  async SetPlaybackRate(double aRate);
> +  async SetVolume(double aVolume);
> +  // Close the HW_CDM, e.g. entering dormant state / closing player.
> +  async ShutDown();

Shut'd'own.

@@ +42,5 @@
> +  // Close the HW_CDM, e.g. entering dormant state / closing player.
> +  async ShutDown();
> +  // Notify CDM to flush all queued data, and not render before ‘time’ param,
> +  // CDM needs to request frame for display after flushing.
> +  async SeekTo(int64_t aSeekToTime);

I can't see how this function makes sense. A renderer should just render the data at hand. All you need is a Flush() function to discard obsolete data so the renderer can play data at new position after seeking.

@@ +44,5 @@
> +  // Notify CDM to flush all queued data, and not render before ‘time’ param,
> +  // CDM needs to request frame for display after flushing.
> +  async SeekTo(int64_t aSeekToTime);
> +  //Notify HW_CDM that MediaDecoder is in seek/pause/play state
> +  async StateChange(GMPMediaPlayState aState);

What is this function for?

@@ +59,5 @@
> +  // Notify last media frame is rendered (EOS encountered)
> +  async AudioRenderReachedEOS();
> +  async VideoRenderReachedEOS();
> +  // Notify parent-side MDSM to request more data and send to child.
> +  // NotifyXXXInputNext() is expected to be called repeatedly after Play(),

Where are NotifyXXXInputNext()s defined?

@@ +72,5 @@
> +  // Notify any error.
> +  async Error(GMPErr aErr);
> +  // MDSM needs to wait CDM resource including this information
> +  //if HW_CDM is gonna do rendering by itself.
> +  async SetOverlayImageID(uint32_t aId);

Is this ID allocated by the parent to tell the child or the other way around?

::: dom/media/gmp/gmp-api/gmp-media-render.h
@@ +42,5 @@
> +#include "gmp-video-codec.h"
> +
> +#include <stdint.h>
> +
> +enum GMPMediaPlayState

What are these states for?

@@ +49,5 @@
> +  kGMPPlay = 1,
> +  kGMPMediaPlayStateInvalid = 2,
> +};
> +
> +enum GMPPacketType

What are these types for?

Comment 54

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

(In reply to James Cheng[:JamesCheng] from comment #40)
> Hi Chris and JW,
> In addition,
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/
> GMPVideoDecoderChild.h?offset=0#36
> 
> (Passing parameter to IPDL-related API)
> It just remains the |const| in the prototype.
> For this case, |const| needs to be removed as you recommended?
> 
> As the result, 
> I should not use |const| anymore for non-IPDL-related code even they will
> pass to the |const&| IPDL API.
> Is that right?
> Thank you.

I agree with JW. |int i| is better than |const int i| for method declarations. Removing the "const"s there for consistency is ok.

Comment 55

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

Comment on attachment 8651604 [details] [diff] [review]
[WIP] GMP-ipdl-implementation.patch

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

::: dom/media/gmp/PGMPMediaRenderer.ipdl
@@ +29,5 @@
> +                          GMPDisplayInfo aDisplayInfo);
> +  //Init Renderer for audio, passing the audio spec to child.
> +  async InitAudioRenderer(GMPAudioCodecData aAudioCodecSettings);
> +  //Bypass the encoded/encrypt video frame to child for rendering.
> +  async RenderVideoPacket(GMPVideoEncodedFrameData aInputFrame,

Re: Sending multiple packets at once to reduce IPC overhead, overall it's probably a good idea, but I think we should *start* with just sending one video packet at a time, and optimize later. Given that this is an internal interface, we can change it at any time.

Each of these frames is backed by a Shmem, which takes up a file descriptor, and we had trouble with running out of available descriptors at one stage. So I think we need to handle optimizing this carefully.

::: dom/media/gmp/gmp-api/gmp-media-render.h
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* Copyright (c) 2011, The WebRTC project authors. All rights reserved.
> + * Copyright (c) 2014, Mozilla

Copyright (c) 2015, Mozilla

:)

@@ +41,5 @@
> +#include "gmp-video-frame-encoded.h"
> +#include "gmp-video-codec.h"
> +
> +#include <stdint.h>
> +

You are sorely lacking in comments in this interface header.

Imagine you're an engineer working at some other company, and you've handed the task of implementing this interface and a deadline. You don't know Gecko. What is critical to know to implement this interface? That should be in comments.

Also note that as this is an *external* interface, third parties outside of Mozilla implement it, so the documentation must be better than for internal headers. (Which is why I'm being harder on this header, and less so on others)

@@ +57,5 @@
> +  kGMPLast = 2,
> +  kGMPPacketTypeInvalid = 3,
> +};
> +
> +struct GMPDisplayInfo

Why don't you just call this GMPRect? That's what it is right, a rectangle?

@@ +70,5 @@
> +{
> +public:
> +  virtual ~GMPMediaRendererCallback() {}
> +
> +  virtual void TimeUpdate(int64_t aElapsedTime) = 0;

Convention for GMP timestamps is for them to be uint64_t. You also need a comment here saying that this timestamp is in microseconds.

@@ +94,5 @@
> +
> +#define GMP_API_MEDIA_RENDERER "render-media"
> +
> +// Media Rendering for a single stream. A GMP may be asked to create multiple
> +// decoders concurrently.

When you say "A GMP may be asked to create multiple decoders concurrently.", do you mean "A GMP may be asked to create multiple GMPMediaRenderers concurrently."?

@@ +105,5 @@
> +{
> +public:
> +  virtual ~GMPMediaRenderer() {}
> +
> +  virtual void InitVideoRenderer(const GMPVideoCodec& aVideoCodecSettings,

Can the GMPMediaRendererCallback used change between InitVideoRenderer calls? What about the number of cores or the DisplayInfo?

What if you want to create one of these for an audio-only stream? Then you'd need to init a video renderer just so that you have a callback.

I think you should have a separate Init() function which takes the Callback, core count, and display info.

@@ +110,5 @@
> +                                 const uint8_t* aVideoCodecSpecific,
> +                                 uint32_t aVideoCodecSpecificLength,
> +                                 GMPMediaRendererCallback* aCallback,
> +                                 int32_t aCoreCount,
> +                                 GMPDisplayInfo aDisplayInfo) = 0;

Why do you pass a DisplayInfo here, and then again in SetDisplayPosition below? Because you don't have comments, I can't figure out why.

Is this the screen size, or the render region?

@@ +114,5 @@
> +                                 GMPDisplayInfo aDisplayInfo) = 0;
> +
> +  virtual void InitAudioRenderer(const GMPAudioCodec& aAudioCodecSettings) = 0;
> +
> +  virtual void RenderVideoPacket(GMPVideoEncodedFrame* aInputFrame,

There are nasty edge cases that we encounter with MediaSource content here:

1. What happens when we switch streams from one quality level to the other, and then the new stream *overlaps* the time renderable by the old stream.
i.e. we're playing video stream 1, and have received time [0, ..., 10], and MSE switches quality and stream 2 inputs [8, ..., 20]. From which stream do you play range [8..10]?

2. What happens when we switch streams from one quality level to the other, and then the new stream doesn't start where the old stream stops?
i.e. we're playing video stream 1, and have received time [0, ..., 10], and MSE switches quality and stream 2 inputs [10.1, ... 20], i.e. we're missing [10.0, 10.1], what should the renderer play for that 0.1 time units?

We've hit both of these cases on YouTube, it's caused by sloppy encodings being input by sites' JavaScript, and we're bound to hit them again. When the rendering is on the Firefox side, we handle this by adding a fuzz factor in the "are ranges contiguous" calculation, and partners implementing this interface will need to be doing something like this too.

I will talk to :jya about this when he gets back from PTO. He may have an idea how we should handle these cases. Probably we'll need a comment in the header here so that implementors know to handle that case.

@@ +125,5 @@
> +                                 GMPPacketType aPacketType) = 0;
> +
> +  virtual void SetPlaybackRate(double aRate) = 0;
> +
> +  virtual void SetVolume(double aVolume) = 0;

What's the min/max volume? You need comments to explain that.

@@ +130,5 @@
> +
> +  //Should callback ShutdownComplete when completing the shutdown task.
> +  virtual void ShutDown() = 0;
> +
> +  virtual void SeekTo(int64_t aSeekToTime) = 0;

uint64_t. 

Comment to say that time is in microseconds.

You also need a comment explaining what the implementation is supposed to do, i.e. do exactly X and then call the callback.

@@ +132,5 @@
> +  virtual void ShutDown() = 0;
> +
> +  virtual void SeekTo(int64_t aSeekToTime) = 0;
> +
> +  virtual void StateChange(GMPMediaPlayState aState) = 0;

"SetState" or "ChangeState".

"StateChange" sounds like you are stating something that has happened (i.e. that this method is a notification/update), rather than ordering something to happen.

Comment 56

[Kilik Kuo [:kikuo] (inactive)]

 > @@ +42,5 @@
> > +  // Close the HW_CDM, e.g. entering dormant state / closing player.
> > +  async ShutDown();
> > +  // Notify CDM to flush all queued data, and not render before ‘time’ param,
> > +  // CDM needs to request frame for display after flushing.
> > +  async SeekTo(int64_t aSeekToTime);
> 
> I can't see how this function makes sense. A renderer should just render the
> data at hand. All you need is a Flush() function to discard obsolete data so
> the renderer can play data at new position after seeking.
> 

As a pure "Renderer", having a "SeekTo" API is indeed weird, I agree !
But it's also insufficient to tell an external consumer that we're doing a seek by only a "Flush" API. Since the "external renderer" here may also act as an external player.  That's why there's a "FlushAndDropUntil($time)" in the previous API version which intends to tell the external renderer what should do next.

I think the goal which we're trying to achieve here is to provide a "MediaDataConsumer" API and not to name it as "Renderer" API.

In that case, we may have several pure interfaces ...

interface GMPMediaDataRenderConsumer      // Provide APIs for rendering media data only.
interface GMPMediaDataPlaybackConsumer    // Provide APIs for playback controlling only.

So if we're rendering decoded data by GMPChild.
Only subclass instance inheriting GMPMediaDataRenderConsumer shall be used in Decoded{Audio,Video}DataSink.

Else if we're sending encrypted data to GMPChild for playback.
A subclass instance inheriting both GMPMediaDataRenderConsumer and GMPMediaDataPlaybackConsumer will be used in Encrypted{Audio,Video}DataSink.

In conclusion, I think originally I was proposing APIs which combine both rendering and controlling.
By separating them into 2 different APIs and combining them by need would be more appropriate,
what do you think ?

Comment 57

[JW Wang [:jwwang] [:jw_wang]]

I failed to see why it has to inherit GMPMediaDataPlaybackConsumer when playing encrypted content. Is it any different from playing clear content?

Comment 58

[Kilik Kuo [:kikuo] (inactive)]

(In reply to JW Wang [:jwwang] from comment #57)
> I failed to see why it has to inherit GMPMediaDataPlaybackConsumer when
> playing encrypted content. Is it any different from playing clear content?

Playing clear content is a renderer's job and the renderer doesn't need to handle A/V synchronization.
But playing encrypted content, the consumer acts like an another state machine. 

By separating "Render" API and "Playback" API could fit both situation and provide a clear state transition for different type of external consumers.

Take SEEK for example, (encrypted data)
If we only tell external consumer to flush the data w/o providing a seek target time, it won't know the correct position to start playback. This could be resolved by "Playback" API via providing a seek timestamp.

Take SEEK-again-before-previous-SEEK-completed for example,
The seek target time could be updated in player content process and wait for the final SeekCompleted($time). Regarding how many times the external consumer calls SeekCompleted($time) back, it will depends on renderer's implementation.

Render API includes data consuming e.g. RenderData($data), InputDataExhausted(), InputEOS(), Flush()/FlushCompleted()...
Playback API includes playback control e.g. Play(), Stop(), Pause(true/false), Seek($time)/SeekCompleted($time)...

Just my two cents.

Comment 59

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #58)
> Playing clear content is a renderer's job and the renderer doesn't need to
> handle A/V synchronization.
A renderer should manage A/V sync on its own no matter it is playing clear or encrypted content.
 
> Take SEEK for example, (encrypted data)
> If we only tell external consumer to flush the data w/o providing a seek
> target time, it won't know the correct position to start playback. This
> could be resolved by "Playback" API via providing a seek timestamp.
See the design of MediaSink::Start() which is told a position from which to start playback.
https://hg.mozilla.org/try/rev/0cb3f06a3ede#l1.97

MediaSink as a abstract audio/video data consumer doesn't have any seek or flush functions which are controlled by MDSM which is effectively a player.

Comment 60

[Kilik Kuo [:kikuo] (inactive)]

> See the design of MediaSink::Start() which is told a position from which to
> start playback.
> https://hg.mozilla.org/try/rev/0cb3f06a3ede#l1.97

Aha ! I see ! The position could be provided when playback starts no matter at the 1st time playback or playback-after-seek.

In addition, considering GMP case, a |StopCompleted()| should be needed to wait until queued data flushing accomplished. And the |StopCompleted()| is different from |ShutdownCompleted()|. 
Good move !

> 
> MediaSink as a abstract audio/video data consumer doesn't have any seek or
> flush functions which are controlled by MDSM which is effectively a player.

Comment 61

[James Cheng[:JamesCheng]]

Thanks for your feedbacks Chris, JW and Kilik.

I will revise my patch according to your suggestions.

Thanks.

Comment 62

[James Cheng[:JamesCheng]]

Attachment: [WIP] Bug-1146796-GMP-ipdl-implementation.patch

According to comment 53, comment 55, comment 60

In summary,

1. I deleted the |ChangeState|, |SeekTo| API and related enum like |GMPPackageType| and |GMPMediaPlayState|. 

Modify |GMPDisplayInfo| into |GMPRect|, timestamps to uint64_t.
Seperate registering callback from InitVideoRenderer(leave core numbers and display region info in this API since core numbers is fixed and display region can be changed by SetDisplayPosition API). Instead, using |SetRendererCallback| for registering the callback independently.

2. Add |SetPreservesPitch|, |Start($time)|, |Stop()| and |StopComplete()| in the API interface for fulfilling the MediaSink interface.

3. Add as much comment as I can in gmp-media-renderer.h.

Thanks for Chris, JW and Kilik's feedbacks.

Please feedback me for more details 

Thank you very much.

Comment 63

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

Update for the feedbacks.

Comment 64

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype.patch

Update for feedbacks.

Comment 65

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

I realised that the MP4 container represents timestamps as signed integers, so we actually need to change the timestamps here back to signed ints. Also other formats may use signed timestamps, so we'd better change them back.

Comment 66

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

Comment on attachment 8656427 [details] [diff] [review]
[WIP] Bug-1146796-GMP-ipdl-implementation.patch

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

Would be good to get jya's feedback on how having rendering performed by a GMP implementing the interface in gmp-media-render.h would interact with MSE.

Comment 67

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

Change the timestamp type back from uint64_t to int64_t.
And fix build error since |FAIL_ON_WARNINGS = True| 

Hi JYA,

These patches is for external rendering APIs, it will interact with AudioSink/VideoSink in the future.

These APIs are designed for passing demuxed-only data to external process for decrypting, decoding and rendering.

Please help me if the API design is sufficient for the current scenario.

Thank you.

Comment 68

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype.patch

rebase and remove FAIL_ON_WARNINGS

Comment 69

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

Rebase and fix timestamp to int64_t

Comment 70

[Josh Cheng [:josh]]

Not in 2016 scope. Move target milestone.

Comment 71

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

For Bug 1194606 Comment 26, we should add an API |ContentChanged| for notifying that the A/V content change(maybe resolution change). Drop |Notify| prefix by comment 34.

Comment 72

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype.patch

Add ContentChanged API for Bug 1194606 Comment 26

Comment 73

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

Add ContentChanged API for Bug 1194606 Comment 26

Comment 74

[James Cheng[:JamesCheng]]

Comment on attachment 8663601 [details] [diff] [review]
[WIP] Add-gmp-renderer-fake-prototype.patch

># HG changeset patch
># User James Cheng <[e-mail]>
>
>Bug 1146796 - Add gmp renderer fake prototype.
>
>
>diff --git a/dom/media/gmp-plugin-renderer/Makefile.in b/dom/media/gmp-plugin-renderer/Makefile.in
>new file mode 100644
>index 0000000..50806331
>--- /dev/null
>+++ b/dom/media/gmp-plugin-renderer/Makefile.in
>@@ -0,0 +1,11 @@
>+#
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+INSTALL_TARGETS += FAKE_GMP_RENDERER_PLUGIN
>+FAKE_GMP_RENDERER_PLUGIN_DEST = $(DEPTH)/dist/bin/gmp-fakerenderer/1.0
>+FAKE_GMP_RENDERER_PLUGIN_FILES = \
>+  $(srcdir)/fakerenderer.info \
>+  $(srcdir)/fakerenderer.voucher \
>+  $(NULL)
>diff --git a/dom/media/gmp-plugin-renderer/fakerenderer.info b/dom/media/gmp-plugin-renderer/fakerenderer.info
>new file mode 100644
>index 0000000..12ebf85
>--- /dev/null
>+++ b/dom/media/gmp-plugin-renderer/fakerenderer.info
>@@ -0,0 +1,4 @@
>+Name: fakerenderer
>+Description: Fake GMP Renderer Plugin
>+Version: 1.0
>+APIs: render-media[external.renderer:fake]
>diff --git a/dom/media/gmp-plugin-renderer/fakerenderer.voucher b/dom/media/gmp-plugin-renderer/fakerenderer.voucher
>new file mode 100644
>index 0000000..0ec70e8
>--- /dev/null
>+++ b/dom/media/gmp-plugin-renderer/fakerenderer.voucher
>@@ -0,0 +1 @@
>+gmp-fakerenderer placeholder voucher
>\ No newline at end of file
>diff --git a/dom/media/gmp-plugin-renderer/gmp-fake-renderer.cpp b/dom/media/gmp-plugin-renderer/gmp-fake-renderer.cpp
>new file mode 100644
>index 0000000..2fb3639
>--- /dev/null
>+++ b/dom/media/gmp-plugin-renderer/gmp-fake-renderer.cpp
>@@ -0,0 +1,249 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include <stdint.h>
>+#include <time.h>
>+#include <cstdio>
>+#include <cstring>
>+#include <iostream>
>+#include <string>
>+#include <memory>
>+#include <assert.h>
>+#include <limits.h>
>+
>+#include "gmp-platform.h"
>+#include "gmp-video-host.h"
>+#include "gmp-video-encode.h"
>+#include "gmp-video-decode.h"
>+#include "gmp-video-frame-i420.h"
>+#include "gmp-video-frame-encoded.h"
>+#include "gmp-media-render.h"
>+
>+#if defined(_MSC_VER)
>+#define PUBLIC_FUNC __declspec(dllexport)
>+#else
>+#define PUBLIC_FUNC
>+#endif
>+
>+static int g_log_level = 4;
>+
>+#define GMPLOG(l, x) do { \
>+        if (l <= g_log_level) { \
>+        const char *log_string = "unknown"; \
>+        if ((l >= 0) && (l <= 3)) {               \
>+        log_string = kLogStrings[l];            \
>+        } \
>+        std::cerr << log_string << ": " << x << std::endl; \
>+        } \
>+    } while(0)
>+
>+#define GL_CRIT 0
>+#define GL_ERROR 1
>+#define GL_INFO  2
>+#define GL_DEBUG 3
>+
>+const char* kLogStrings[] = {
>+  "Critical",
>+  "Error",
>+  "Info",
>+  "Debug"
>+};
>+
>+
>+GMPPlatformAPI* g_platform_api = NULL;
>+
>+class FakeMediaRenderer;
>+
>+class FakeRenderer;
>+class FakeRendererVideoTask : public GMPTask {
>+ public:
>+  FakeRendererVideoTask(FakeMediaRenderer* renderer,
>+                        GMPVideoEncodedFrame* frame)
>+    : renderer_(renderer), frame_(frame) {}
>+
>+  virtual void Run();
>+  virtual void Destroy() { delete this; }
>+
>+  FakeMediaRenderer* renderer_;
>+  GMPVideoEncodedFrame* frame_;
>+  int64_t time_;
>+};
>+
>+class FakeRendererAudioTask : public GMPTask {
>+ public:
>+  FakeRendererAudioTask(FakeMediaRenderer* renderer,
>+                        GMPAudioSamples* sample)
>+    : renderer_(renderer), audioSample_(sample) {}
>+
>+  virtual void Run();
>+  virtual void Destroy() { delete this; }
>+
>+  FakeMediaRenderer* renderer_;
>+  GMPAudioSamples* audioSample_;
>+};
>+
>+class FakeMediaRenderer : public GMPMediaRenderer {
>+ public:
>+  explicit FakeMediaRenderer (GMPVideoHost* hostAPI) :
>+    host_ (hostAPI),
>+    callback_ (NULL) {}
>+
>+  virtual ~FakeMediaRenderer() {
>+  }
>+
>+  virtual void InitVideoRenderer(const GMPVideoCodec& aVideoCodecSettings,
>+                                 const uint8_t* aCodecSpecific,
>+                                 uint32_t aVideoCodecSpecificLength,
>+                                 int32_t aCoreCount,
>+                                 GMPRect aDisplayRect)
>+  {
>+    GMPLOG (GL_DEBUG, "InitVideoRenderer");
>+  }
>+
>+  virtual void InitAudioRenderer(const GMPAudioCodec& aAudioCodecSettings)
>+  {
>+    GMPLOG (GL_DEBUG, "InitAudioRenderer");
>+  }
>+
>+  virtual void SetRendererCallback(GMPMediaRendererCallback* aCallback)
>+  {
>+    GMPLOG (GL_DEBUG, "SetRendererCallback");
>+
>+    callback_ = aCallback;
>+
>+    callback_->SetOverlayImageID(1);
>+  }
>+
>+  virtual void RenderVideoPacket(GMPVideoEncodedFrame* aInputFrame,
>+                                 bool aMissingFrames,
>+                                 const uint8_t* aCodecSpecificInfo,
>+                                 uint32_t aCodecSpecificInfoLength)
>+  {
>+    GMPLOG (GL_DEBUG, "RenderVideoPacket");
>+    g_platform_api->runonmainthread(new FakeRendererVideoTask(this, aInputFrame));
>+  }
>+
>+  virtual void RenderAudioPacket(GMPAudioSamples* aEncodedSamples)
>+  {
>+    GMPLOG (GL_DEBUG, "RenderAudioPacket");
>+    g_platform_api->runonmainthread(new FakeRendererAudioTask(this, aEncodedSamples));
>+  }
>+
>+  virtual void SetPlaybackRate(double aRate)
>+  {
>+    GMPLOG (GL_DEBUG, "SetPlaybackRate");
>+  }
>+
>+  virtual void SetVolume(double aVolume)
>+  {
>+    GMPLOG (GL_DEBUG, "SetVolume");
>+  }
>+
>+  virtual void SetPreservesPitch(bool aPreservesPitch)
>+  {
>+    GMPLOG (GL_DEBUG, "SetPreservesPitch");
>+  }
>+
>+  virtual void Shutdown()
>+  {
>+    GMPLOG (GL_DEBUG, "Shutdown");
>+    //PerformShutdown...
>+
>+    GMPLOG (GL_DEBUG, "\033[1;31m============ShutdownComplete============\n\033[m");
>+    callback_->ShutdownComplete();
>+  }
>+
>+  virtual void Start(int64_t aStartTime)
>+  {
>+    GMPLOG (GL_DEBUG, "Start");
>+    //Do something for the start command.
>+    //...................
>+    //Then callback for the a/v frame.
>+    callback_->VideoInputDataExhausted();
>+    callback_->AudioInputDataExhausted();
>+  }
>+
>+  virtual void Stop()
>+  {
>+    GMPLOG (GL_DEBUG, "Stop");
>+    //Do something for the stop command.
>+    callback_->StopComplete();
>+  }
>+
>+  virtual void ContentChanged(int64_t aTime)
>+  {
>+    GMPLOG (GL_DEBUG, "ContentChanged");
>+    //Do something for the ContentChanged command.
>+    //...................
>+    //Do some logic for maybe resolution changed for the next incoming samples.
>+    //Then callback for the a/v frame for the new content.
>+    callback_->VideoInputDataExhausted();
>+    callback_->AudioInputDataExhausted();
>+  }
>+
>+  virtual void SetDisplayPosition(GMPRect aDisplayRect)
>+  {
>+    GMPLOG (GL_DEBUG, "SetDisplayPosition");
>+  }
>+
>+  void SimulateRenderVideo(GMPVideoEncodedFrame* inputFrame)
>+  {
>+    GMPLOG (GL_DEBUG, "\033[1;31m============SimulateRenderVideo============\n\033[m");
>+    //Perform external video rendering...
>+
>+    //callback to parent for acquiring next video frame.
>+    callback_->VideoInputDataExhausted();
>+  }
>+
>+  void SimulateRenderAudio(GMPAudioSamples* audioSample)
>+  {
>+    GMPLOG (GL_DEBUG, "\033[1;31m============SimulateRenderAudio============\n\033[m");
>+    //Perform external audio rendering...
>+
>+    //callback to parent for acquiring next audio sample.
>+    callback_->AudioInputDataExhausted();
>+  }
>+  GMPVideoHost* host_;
>+  GMPMediaRendererCallback* callback_;
>+};
>+
>+void FakeRendererVideoTask::Run() {
>+  renderer_->SimulateRenderVideo(frame_);
>+  frame_->Destroy();
>+}
>+
>+void FakeRendererAudioTask::Run() {
>+  renderer_->SimulateRenderAudio(audioSample_);
>+  audioSample_->Destroy();
>+}
>+
>+extern "C" {
>+
>+  PUBLIC_FUNC GMPErr
>+  GMPInit (GMPPlatformAPI* aPlatformAPI) {
>+    GMPLOG (GL_DEBUG, "GMPInit");
>+    g_platform_api = aPlatformAPI;
>+    return GMPNoErr;
>+  }
>+
>+  PUBLIC_FUNC GMPErr
>+  GMPGetAPI (const char* aApiName, void* aHostAPI, void** aPluginApi) {
>+    GMPLOG (GL_DEBUG,
>+            __FUNCTION__
>+            << " aApiName="
>+            << aApiName);
>+    if (!strcmp (aApiName, GMP_API_MEDIA_RENDERER)) {
>+      *aPluginApi = new FakeMediaRenderer (static_cast<GMPVideoHost*> (aHostAPI));
>+      return GMPNoErr;
>+    }
>+    return GMPGenericErr;
>+  }
>+
>+  PUBLIC_FUNC void
>+  GMPShutdown (void) {
>+    g_platform_api = NULL;
>+  }
>+
>+} // extern "C"
>diff --git a/dom/media/gmp-plugin-renderer/moz.build b/dom/media/gmp-plugin-renderer/moz.build
>new file mode 100644
>index 0000000..7c25f1a
>--- /dev/null
>+++ b/dom/media/gmp-plugin-renderer/moz.build
>@@ -0,0 +1,24 @@
>+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
>+# vim: set filetype=python:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+# largely a copy of dom/media/gmp-fake/moz.build
>+
>+FINAL_TARGET = 'dist/bin/gmp-fakerenderer/1.0'
>+SOURCES += [
>+    'gmp-fake-renderer.cpp',
>+]
>+
>+SharedLibrary("fakerenderer")
>+
>+if CONFIG['OS_ARCH'] == 'WINNT':
>+    OS_LIBS += [
>+        'ole32',
>+    ]
>+
>+USE_STATIC_LIBS = True
>+NO_VISIBILITY_FLAGS = True
>+# Don't use STL wrappers; this isn't Gecko code
>+DISABLE_STL_WRAPPING = True
>diff --git a/dom/media/moz.build b/dom/media/moz.build
>index 8125062..a3d618c 100644
>--- a/dom/media/moz.build
>+++ b/dom/media/moz.build
>@@ -19,16 +19,17 @@ component_av = ('Core', 'WebRTC: Audio/Video')
> with Files('GetUserMedia*'):
>     BUG_COMPONENT = component_av
> 
> DIRS += [
>     'encoder',
>     'gmp',
>     'gmp-plugin',
>     'gmp-plugin-openh264',
>+    'gmp-plugin-renderer',
>     'imagecapture',
>     'mediasink',
>     'mediasource',
>     'ogg',
>     'platforms',
>     'systemservices',
>     'webaudio',
>     'webrtc',
>

Comment 75

[James Cheng[:JamesCheng]]

Attachment: [WIP] config-for-adding-fake-gmp-renderer.patch

rebase to current code base.

Comment 76

[James Cheng[:JamesCheng]]

Attachment: [WIP] Enable-GMP-Fake-Renderer-on-b2g.patch

rebase

Comment 77

[James Cheng[:JamesCheng]]

Attachment: [WIP] GMP-ipdl-implementation.patch

rebase

Comment 78

[James Cheng[:JamesCheng]]

Attachment: [WIP] Add-gmp-renderer-fake-prototype.patch

rebase

Comment 79

[James Cheng[:JamesCheng]]

Attachment: [WIP] Testing-code-for-initialize-the-GMPMedia.patch

rebase

Comment 80

[James Cheng[:JamesCheng]]

Attachment: [WIP] named-pipe-on-gmp-fake-renderer.patch

testing code for communicate with other process by named pipe.

For testing, need to add MOZ_DISABLE_GMP_SANDBOX=1 ./mach run when using pipe.

Comment 81

[James Cheng[:JamesCheng]]

Attachment: pipehandler.tar.gz

upload a test client for handling the gmp-fake renderer pipe command.

Comment 82

[BMO Automation]

Firefox OS is not being worked on