Closed Bug 999704 Opened 6 years ago Closed 6 years ago

WebRTC Gecko Media Plugins adapter

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: shell, Assigned: ekr)

References

()

Details

(Whiteboard: p=.25, s=fx32, c=webrtc])

Attachments

(8 files, 10 obsolete files)

10.90 KB, patch
Details | Diff | Splinter Review
6.63 KB, patch
Details | Diff | Splinter Review
10.19 KB, patch
jesup
: review-
Details | Diff | Splinter Review
20.17 KB, patch
Details | Diff | Splinter Review
5.88 KB, patch
Details | Diff | Splinter Review
57.60 KB, patch
ehugg
: review+
jesup
: review+
Details | Diff | Splinter Review
7.13 KB, text/html
Details
1.58 KB, patch
pkerr
: review+
Details | Diff | Splinter Review
written but not landed and not ready for review. [EKR, Lands next]
Assignee: nobody → ekr
p=.25 is a placeholder, to find bugs that need estimates - but be able to track in Scrumbugs if we know timeframe for landing. https://scrumbu.gs/t/webrtc-platform/fx32/
Whiteboard: p=.25, s=fx32, c=webrtc]
The description of what this bug covers or what the written-but-not-ready-to-land patch does is rather slight.  I presume this bug covers interfacing anything loaded with the GMP API to WebRTC.
Attached patch [PATCH] Make OpenH264 work (obsolete) — Splinter Review
---
 content/media/gmp/GMPChild.cpp                     |   2 +
 content/media/gmp/GMPProcessParent.cpp             |   3 +-
 media/webrtc/signaling/signaling.gyp               |   5 +
 .../signaling/src/media-conduit/CodecConfig.h      |   1 -
 .../signaling/src/media-conduit/FakeVideoCodec.cpp |  22 +
 .../signaling/src/media-conduit/FakeVideoCodec.h   |  14 +
 .../signaling/src/media-conduit/GmpVideoCodec.cpp  |  22 +
 .../signaling/src/media-conduit/GmpVideoCodec.h    |  19 +
 .../src/media-conduit/MediaConduitInterface.h      |   1 -
 .../signaling/src/media-conduit/VideoConduit.cpp   |  99 +++++
 .../signaling/src/media-conduit/VideoConduit.h     |  16 +-
 .../src/media-conduit/WebrtcFakeVideoCodec.cpp     | 193 +++++++++
 .../src/media-conduit/WebrtcFakeVideoCodec.h       | 113 +++++
 .../src/media-conduit/WebrtcGmpVideoCodec.cpp      | 478 +++++++++++++++++++++
 .../src/media-conduit/WebrtcGmpVideoCodec.h        | 183 ++++++++
 .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |  21 +-
 .../src/peerconnection/PeerConnectionCtx.cpp       |   3 +-
 .../signaling/test/mediaconduit_unittests.cpp      | 248 ++++++++---
 .../modules/video_capture/video_capture_impl.cc    |   3 +
 .../video_coding/main/source/codec_database.cc     |  12 +
 .../video_coding/main/source/video_sender.cc       |  23 +-
 .../webrtc/system_wrappers/interface/scoped_ptr.h  |  19 +-
 .../trunk/webrtc/video_engine/vie_capturer.cc      |   4 +
 .../trunk/webrtc/video_engine/vie_encoder.cc       |  16 +
 24 files changed, 1442 insertions(+), 78 deletions(-)
 create mode 100644 media/webrtc/signaling/src/media-conduit/FakeVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/FakeVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/GmpVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/GmpVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
Attached patch [PATCH] Make OpenH264 work (obsolete) — Splinter Review
---
 media/webrtc/signaling/signaling.gyp               |   5 +
 .../signaling/src/media-conduit/FakeVideoCodec.cpp |  22 +
 .../signaling/src/media-conduit/FakeVideoCodec.h   |  14 +
 .../signaling/src/media-conduit/GmpVideoCodec.cpp  |  22 +
 .../signaling/src/media-conduit/GmpVideoCodec.h    |  19 +
 .../signaling/src/media-conduit/VideoConduit.cpp   |  99 +++++
 .../signaling/src/media-conduit/VideoConduit.h     |  16 +-
 .../src/media-conduit/WebrtcFakeVideoCodec.cpp     | 193 +++++++++
 .../src/media-conduit/WebrtcFakeVideoCodec.h       | 113 +++++
 .../src/media-conduit/WebrtcGmpVideoCodec.cpp      | 478 +++++++++++++++++++++
 .../src/media-conduit/WebrtcGmpVideoCodec.h        | 183 ++++++++
 .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |  21 +-
 .../signaling/test/mediaconduit_unittests.cpp      | 248 ++++++++---
 .../webrtc/system_wrappers/interface/scoped_ptr.h  |  19 +-
 14 files changed, 1383 insertions(+), 69 deletions(-)
 create mode 100644 media/webrtc/signaling/src/media-conduit/FakeVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/FakeVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/GmpVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/GmpVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
Attachment #8420586 - Attachment is obsolete: true
Attached is a WIP patch. Ignore the scoped_ptr.h diff, which is spurious.
Depends on: 957928
Comment on attachment 8420587 [details] [diff] [review]
[PATCH] Make OpenH264 work

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

nits.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +773,5 @@
>  
>        if (branch) {
>  	branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable);
> +
> +        branch->GetBoolPref("media.video.frame_metrics", &mFrameMetrics);

This needs to be acquired in the send codec, not the receive codec.

::: media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h
@@ -21,3 @@
>  
>  //  release() added in by Google. Use this to conditionally
>  //  transfer ownership of a heap-allocated object to the caller, usually on

Revert changes to this file
Attached patch [PATCH] Make OpenH264 work (obsolete) — Splinter Review
---
 media/webrtc/signaling/signaling.gyp               |   5 +
 .../signaling/src/media-conduit/FakeVideoCodec.cpp |  22 +
 .../signaling/src/media-conduit/FakeVideoCodec.h   |  14 +
 .../signaling/src/media-conduit/GmpVideoCodec.cpp  |  22 +
 .../signaling/src/media-conduit/GmpVideoCodec.h    |  19 +
 .../signaling/src/media-conduit/VideoConduit.cpp   | 114 ++++-
 .../signaling/src/media-conduit/VideoConduit.h     |  16 +-
 .../src/media-conduit/WebrtcFakeVideoCodec.cpp     | 193 +++++++++
 .../src/media-conduit/WebrtcFakeVideoCodec.h       | 113 +++++
 .../src/media-conduit/WebrtcGmpVideoCodec.cpp      | 478 +++++++++++++++++++++
 .../src/media-conduit/WebrtcGmpVideoCodec.h        | 183 ++++++++
 .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |  21 +-
 .../signaling/test/mediaconduit_unittests.cpp      | 248 ++++++++---
 13 files changed, 1387 insertions(+), 61 deletions(-)
 create mode 100644 media/webrtc/signaling/src/media-conduit/FakeVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/FakeVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/GmpVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/GmpVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.h
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
 create mode 100644 media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
Attachment #8420587 - Attachment is obsolete: true
I check the encoded bit stream in the WebrtcGmpVideoEncoder::Encoded() function before this callback:
+  callback_->Encoded(image, nullptr, nullptr);

and found the buffer includes the NAL Start Code {0x00, 0x00, 0x00, 0x01}
It could conflict the design of WebrtcOMXDecoder.
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp#317
It will also append the start code before request omx decoder to decode that.

Do we want to handle this on WebrtcGmpVideoCodec or WebrtcOMXH264VideoCodec?
(In reply to Randy Lin [:rlin] from comment #9)
> I check the encoded bit stream in the WebrtcGmpVideoEncoder::Encoded()
> function before this callback:
> +  callback_->Encoded(image, nullptr, nullptr);
> 
> and found the buffer includes the NAL Start Code {0x00, 0x00, 0x00, 0x01}
> It could conflict the design of WebrtcOMXDecoder.
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> media-conduit/WebrtcOMXH264VideoCodec.cpp#317
> It will also append the start code before request omx decoder to decode that.
> 
> Do we want to handle this on WebrtcGmpVideoCodec or WebrtcOMXH264VideoCodec?

We should probably refactor this so that we have common NAL->RTP mapping
code.
Note that the H264 packetization patches cause start-code stuffing into data shipped down for decode.  So my patch queue for OMX work removes the start-code stuffing currently there.
Note that the H264 mode 1 packetization patches have landed, and the H264 OMX code now is expecting start codes to be stuffed by the packetization code.
Slight modify https://bugzilla.mozilla.org/attachment.cgi?id=8420600 and make it can connect with flame.

Changes: 
Separate the encoded frame with multiple NAL Units into single one and callback individually.
Duplicate of this bug: 996379
From rlin's patch - cleaned up, stats and frame timestamping moved to separate patches; style moved to mozilla style.  Added H264 negotiation params (still unused).  Needs definition of the buffer API for data coming from OpenH264 (multiple NALs).  Probably good enough for preffed-off landing with followups filed
this may belong on a different bug...
Comment on attachment 8433149 [details] [diff] [review]
Implement GMP codec interface to webrtc

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

The FakeVideoCodecs aren't being used here.  At minimum, I'm splitting them into a separate patch

::: content/media/gmp/gmp-api/gmp-video-codec.h
@@ +87,5 @@
> +    // These are null/0 if not externally negotiated
> +    const uint8_t* spsData;
> +    size_t         spsLen;
> +    const uint8_t* ppsData;
> +    size_t         ppsLen;

These should all be mozilla style (cut and paste)

::: media/webrtc/signaling/signaling.gyp
@@ +93,5 @@
>          './src/common/NullDeleter.h',
>          './src/common/Wrapper.h',
>          './src/common/NullTransport.h',
>          './src/common/YuvStamper.cpp',
> +

remove added blank lines

::: media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.cpp
@@ +20,5 @@
> +// Encoder.
> +WebrtcFakeVideoEncoder::WebrtcFakeVideoEncoder()
> +  : timestamp_(0),
> +    callback_(nullptr),
> +    mutex_("WebrtcFakeVideoEncoder") {

mozilla style.  Note: the fake video codecs aren't being used....

::: media/webrtc/signaling/src/media-conduit/WebrtcFakeVideoCodec.h
@@ +32,5 @@
> +struct EncodedFrame {
> +  uint32_t width_;
> +  uint32_t height_;
> +  uint8_t value_;
> +  uint32_t timestamp_;

Mozilla style
also cleaned up includes
Attachment #8433149 - Attachment is obsolete: true
Attachment #8433150 - Attachment is obsolete: true
Attachment #8433152 - Attachment is obsolete: true
Comment on attachment 8433355 [details] [diff] [review]
Implement GMP codec interface to webrtc

derf - if you don't have time to review this today or maybe tomorrow, ping me and remove yourself.  I want you to be involved in the design/impl since we may use this for Daala.  Note that the current patch isn't intended to be a final API, but a temporary one for a preffed-off checkin for 32, with the goal of resolving the design issues and any implementation todo's for 33.

We'll also want to think about future internal vs. external vs. hardware codec control/negotiation, and how packetization/etc is done, and how all this interfaces to webrtc.org external codec api's (which are incomplete today), and interfaces to streaming decode.  That's not really a focus for this review though.  Thanks!
Attachment #8433355 - Flags: review?(tterribe)
Comment on attachment 8433358 [details] [diff] [review]
Implement video frame timestamping VideoConduit

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

We should at minimum put everything in one struct and add an auth tag of some sort.  See what we're doing in the MediaEngineDefault fake video code.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +35,5 @@
> +
> +struct VideoMetaData {
> +  uint32_t magic;
> +  uint32_t frame_ct;
> +  uint32_t timestamp;

mozilla style
Why aren't delta and mSentFrames inside this?
It needs a checksum/CRC/sha/etc tag to avoid corruption or accidental trigger

@@ +406,5 @@
>      }
>    }
>  
> +  mSentFrames = 0;
> +

remove blank line

@@ +629,5 @@
> +    if (NS_SUCCEEDED(rv)) {
> +      nsCOMPtr<nsIPrefBranch> branch = do_QueryInterface(prefs);
> +
> +      if (branch) {
> +        branch->GetBoolPref("media.video.frame_metrics", &mFrameMetrics);

Perhaps this should use the newer static prefs api

@@ +1001,5 @@
>                                     uint64_t capture_time)
>  {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
> +

remove

@@ +1024,5 @@
> +
> +        VideoMetaData meta;
> +        meta.magic = htonl(VIDEO_META_DATA_MAGIC);
> +        meta.frame_ct = htonl(mSentFrames);
> +        uint32_t now_abs = PR_Now() & 0xffffffff;

This will cause issues at rollover time, depending on how it's used.

@@ +1202,5 @@
>  {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
>  
> +

remove
Attachment #8433358 - Flags: review-
Attachment #8420600 - Attachment is obsolete: true
Attachment #8429724 - Attachment is obsolete: true
(In reply to Randell Jesup [:jesup] from comment #23)
> derf - if you don't have time to review this today or maybe tomorrow, ping
> me and remove yourself.  I want you to be involved in the design/impl since

I definitely do not have time today. Depending on how much I manage to get through today, I'll be able to give you a better answer about tomorrow tomorrow.
Comment on attachment 8433358 [details] [diff] [review]
Implement video frame timestamping VideoConduit

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +35,5 @@
> +
> +struct VideoMetaData {
> +  uint32_t magic;
> +  uint32_t frame_ct;
> +  uint32_t timestamp;

> It needs a checksum/CRC/sha/etc tag to avoid corruption or accidental trigger

"magic" avoids accidental trigger.

frame_ct is mSentFrames.

delta is computed locally.
Comment on attachment 8433355 [details] [diff] [review]
Implement GMP codec interface to webrtc

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

::: content/media/gmp/gmp-api/gmp-video-codec.h
@@ +75,5 @@
>    int32_t mKeyFrameInterval;
>  };
>  
> +// H264 specific
> +struct GMPVideoCodecH264

FYI, what's defined here is does everything we need for EME H.264 Decoding via GMP.
(In reply to Eric Rescorla (:ekr) from comment #26)
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +35,5 @@
> > +
> > +struct VideoMetaData {
> > +  uint32_t magic;
> > +  uint32_t frame_ct;
> > +  uint32_t timestamp;
> 
> > It needs a checksum/CRC/sha/etc tag to avoid corruption or accidental trigger
> 
> "magic" avoids accidental trigger.
> 
> frame_ct is mSentFrames.
> 
> delta is computed locally.

Ok, but the code doesn't checksum across the data, and even though there's a magic value to match on, a bit-error in the encoded data wouldn't be caught at all.  Video codecs can twiddle the values (and packet losses even more so) such that the data is corrupted, especially at very low bitrates.  Perhaps this is very unlikely or virtually impossible - but a checksum/CRC/etc is cheap insurance.  We used it in the fake:true video generator.
Comment on attachment 8433355 [details] [diff] [review]
Implement GMP codec interface to webrtc

Adding ekr to reviewers.  I still want derf's if/when possible, and to discuss longer-term API here
Attachment #8433355 - Flags: review?(ekr)
(In reply to Randell Jesup [:jesup] from comment #28)
> (In reply to Eric Rescorla (:ekr) from comment #26)
> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > @@ +35,5 @@
> > > +
> > > +struct VideoMetaData {
> > > +  uint32_t magic;
> > > +  uint32_t frame_ct;
> > > +  uint32_t timestamp;
> > 
> > > It needs a checksum/CRC/sha/etc tag to avoid corruption or accidental trigger
> > 
> > "magic" avoids accidental trigger.
> > 
> > frame_ct is mSentFrames.
> > 
> > delta is computed locally.
> 
> Ok, but the code doesn't checksum across the data, and even though there's a
> magic value to match on, a bit-error in the encoded data wouldn't be caught
> at all.

Yes. As I said, it avoids accidental trigger.


>  Video codecs can twiddle the values (and packet losses even more
> so) such that the data is corrupted, especially at very low bitrates. 
> Perhaps this is very unlikely or virtually impossible - but a
> checksum/CRC/etc is cheap insurance.  We used it in the fake:true video
> generator.

I don't object to adding a checksum, but I don't think it's necessary before
this patch lands. I would just add a reference to another bug.
Comment on attachment 8433355 [details] [diff] [review]
Implement GMP codec interface to webrtc

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

I'm the original author of this code, so I think we should find someone
else to review it.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +81,5 @@
>    }
>  
> +   if (mPtrExtCodec) {
> +     mPtrExtCodec->Release();
> +     mPtrExtCodec = NULL;

nullptr.
Attachment #8433355 - Flags: review?(ekr)
Attachment #8433355 - Flags: review?(joshmoz)
Comment on attachment 8433355 [details] [diff] [review]
Implement GMP codec interface to webrtc

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

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

Make this a fixed-size integer, int32_t or whatever you want.

::: media/webrtc/signaling/src/media-conduit/GmpVideoCodec.cpp
@@ +9,5 @@
> +
> +VideoEncoder* GmpVideoCodec::CreateEncoder() {
> +  WebrtcGmpVideoEncoder *enc =
> +      new WebrtcGmpVideoEncoder();
> +  return enc;

Just return the result of new, no need for the stack variable. Same for CreateDecoder.

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +24,5 @@
> +namespace mozilla {
> +
> +// Encoder.
> +WebrtcGmpVideoEncoder::WebrtcGmpVideoEncoder() :
> +    mMPS(nullptr),

No need to initialize nsCOMPtr.

@@ +28,5 @@
> +    mMPS(nullptr),
> +    mGMPThread(nullptr),
> +    mGMP(nullptr),
> +    mHost(nullptr),
> +    mCallback(nullptr) {}

Style is:

Foo::Bar()
  : mXXX
  , mYYY
  , mZZZ
{
}

@@ +34,5 @@
> +static int
> +WebrtcFrameTypeToGmpFrameType(webrtc::VideoFrameType aIn,
> +                              GMPVideoFrameType *aOut)
> +{
> +  // XXX Array lookup?

Best to address this, make the comment more clear, or remove it.

@@ +37,5 @@
> +{
> +  // XXX Array lookup?
> +  switch(aIn) {
> +    case webrtc::kKeyFrame:
> +      *aOut = kGMPKeyFrame;

MOZ_ASSERT(aOut) at the top of the method, also in GmpFrameTypeToWebrtcFrameType.

@@ +63,5 @@
> +static int
> +GmpFrameTypeToWebrtcFrameType(GMPVideoFrameType aIn,
> +                              webrtc::VideoFrameType *aOut)
> +{
> +  // XXX Array lookup?

Same here.

@@ +89,5 @@
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +
> +int32_t WebrtcGmpVideoEncoder::InitEncode(const webrtc::VideoCodec* aCodecSettings,

Put return type on its own line.

@@ +97,5 @@
> +  mMPS = do_GetService("@mozilla.org/gecko-media-plugin-service;1");
> +
> +  if (!mMPS) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }

MOZ_ASSERT(mMPS) instead. This should never fail.

@@ +134,5 @@
> +  mHost = host;
> +
> +  if (!gmp || !host) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }

Check this before assigning to mGMP and mHost?

@@ +136,5 @@
> +  if (!gmp || !host) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  // TODO(ekr@rtfm.com): transfer settings from codecSettings to codec.

Does something about this still need to be addressed? Please do so or remove the comment.

@@ +160,5 @@
> +int32_t
> +WebrtcGmpVideoEncoder::Encode(const webrtc::I420VideoFrame& aInputImage,
> +                              const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
> +                              const std::vector<webrtc::VideoFrameType>* aFrameTypes)
> +{

MOZ_ASSERT(mGMPThread)

@@ +178,5 @@
> +int32_t
> +WebrtcGmpVideoEncoder::Encode_g(const webrtc::I420VideoFrame* aInputImage,
> +                                const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
> +                                const std::vector<webrtc::VideoFrameType>* aFrameTypes)
> +{

MOZ_ASSERT(mHost);
MOZ_ASSERT(mGMP);

@@ +180,5 @@
> +                                const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
> +                                const std::vector<webrtc::VideoFrameType>* aFrameTypes)
> +{
> +  GMPVideoFrame* ftmp = nullptr;
> +  // Translate the image.

Comment seems pointless.

@@ +204,5 @@
> +  }
> +  frame->SetTimestamp(aInputImage->timestamp());
> +  frame->SetRenderTime_ms(aInputImage->render_time_ms());
> +
> +  // TODO(ekr@rtfm.com): actually translate this.

Address this if it needs to be addressed?

@@ +252,5 @@
> +}
> +
> +int32_t
> +WebrtcGmpVideoEncoder::SetRates(uint32_t aNewBitRate, uint32_t aFrameRate)
> +{

MOZ_ASSERT(mGMPThread);

@@ +265,5 @@
> +}
> +
> +int32_t
> +WebrtcGmpVideoEncoder::SetRates_g(uint32_t aNewBitRate, uint32_t aFrameRate)
> +{

MOZ_ASSERT(mGMP);

@@ +284,5 @@
> +WebrtcGmpVideoEncoder::GetNextNALUnit(const uint8_t **aData,
> +                                      size_t *aSize,
> +                                      const uint8_t **aNalStart,
> +                                      size_t *aNalSize,
> +                                      bool aStartCodeFollows) // always true?

Now might be a good time to figure out if aStartCodeFollows is always true.

@@ +285,5 @@
> +                                      size_t *aSize,
> +                                      const uint8_t **aNalStart,
> +                                      size_t *aNalSize,
> +                                      bool aStartCodeFollows) // always true?
> +{

Do you want to assert re: input parameters before you deref them in this method?

@@ +321,5 @@
> +    while (offset < size && data[offset] != 0x01) {
> +      ++offset;
> +    }
> +
> +    // XXX this deserves at least a comment

Replace with proper comment.

@@ +350,5 @@
> +  if (offset + 2 < size) {
> +    *aData = &data[offset - 2];
> +    *aSize = size - offset + 2;
> +  } else {
> +    *aData = NULL;

nullptr

@@ +373,5 @@
> +  const uint8_t* data = aEncodedFrame->Buffer();
> +  size_t size = aEncodedFrame->Size();
> +  const uint8_t* nalStart = nullptr;
> +  size_t nalSize = 0;
> +  // XXX Why always true for GetNextNALUnit?

Resolve this?

@@ +380,5 @@
> +    nalu._length = nalSize;
> +    nalu._frameType = ft;
> +    nalu._timeStamp = aEncodedFrame->TimeStamp();
> +    nalu._completeFrame = true;
> +    mCallback->Encoded(nalu, nullptr, nullptr);

If you absolutely require mCallback to be set at this point, you should MOZ_ASSERT(mCallback) at the top of the method. Otherwise you need to null check it before calling.

@@ +389,5 @@
> +
> +
> +// Decoder.
> +WebrtcGmpVideoDecoder::WebrtcGmpVideoDecoder() :
> +  mMPS(nullptr),

No need to init nsCOMPtr.

@@ +393,5 @@
> +  mMPS(nullptr),
> +  mGMPThread(nullptr),
> +  mGMP(nullptr),
> +  mHost(nullptr),
> +  mCallback(nullptr) {}

Fix constructor style as requested for the encoder version of this.

@@ +402,5 @@
> +{
> +  mMPS = do_GetService("@mozilla.org/gecko-media-plugin-service;1");
> +  if (!mMPS) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }

Just assert this instead, as requested for the encoder version of this method.

@@ +436,5 @@
> +  mGMP = gmp;
> +  mHost = host;
> +  if (!gmp || !host) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }

Check this before assigning to mGMP and mHost?

@@ +438,5 @@
> +  if (!gmp || !host) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  // TODO(ekr@rtfm.com): transfer settings from aCodecSettings to codec.

Does something about this still need to be addressed? Please do so or remove the comment.

@@ +442,5 @@
> +  // TODO(ekr@rtfm.com): transfer settings from aCodecSettings to codec.
> +  GMPVideoCodec codec;
> +  memset(&codec, 0, sizeof(codec));
> +
> +  GMPVideoErr err = mGMP->InitDecode(codec, this, 1); // XXX 1?

The "XXX 1" comment refers to the core count. I'd resolve what you want to do here and get rid of the alarming comment.

@@ +456,5 @@
> +                              bool aMissingFrames,
> +                              const webrtc::RTPFragmentationHeader* aFragmentation,
> +                              const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
> +                              int64_t aRenderTimeMs)
> +{

MOZ_ASSERT(mGMPThread);

@@ +477,5 @@
> +                                bool aMissingFrames,
> +                                const webrtc::RTPFragmentationHeader* aFragmentation,
> +                                const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
> +                                int64_t aRenderTimeMs)
> +{

MOZ_ASSERT(mHost);
MOZ_ASSERT(mGMP);

@@ +503,5 @@
> +  if (ret != WEBRTC_VIDEO_CODEC_OK) {
> +    return ret;
> +  }
> +
> +  // TODO(ekr@rtfm.com): Fill in

Address this?

@@ +557,5 @@
> +  }
> +  image.set_timestamp(aDecodedFrame->Timestamp());
> +  image.set_render_time_ms(0);
> +
> +  mCallback->Decoded(image);

If you absolutely require mCallback to be set at this point, you should MOZ_ASSERT(mCallback) at the top of the method. Otherwise you need to null check it before calling.

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
@@ +45,5 @@
> +                              public GMPEncoderCallback
> +{
> +public:
> +  WebrtcGmpVideoEncoder();
> +  virtual ~WebrtcGmpVideoEncoder() {}

This class should probably not be subclassed so make it MOZ_FINAL in the class declaration, then the dtor doesn't need to be virtual. If this class is refcounted, you can make the dtor private as well.

Same goes for the decoder equivalent.

@@ +69,5 @@
> +                           uint32_t aFrameRate);
> +
> +  // GMPEncoderCallback virtual functions.
> +  virtual void Encoded(GMPVideoEncodedFrame* aEncodedFrame,
> +                       const GMPCodecSpecificInfo& aCodecSpecificInfo);

Needs MOZ_OVERRIDE

@@ +117,5 @@
> +  virtual int32_t Release();
> +
> +  virtual int32_t Reset();
> +
> +  virtual void Decoded(GMPVideoi420Frame* aDecodedFrame);

Add MOZ_OVERRIDE to this and all other GMPDecoderCallback method declarations below.
Attachment #8433355 - Flags: review?(joshmoz)
Updated patch will address pretty much all the comments.

(In reply to Josh Aas (Mozilla Corporation) from comment #32)
> @@ +284,5 @@
> > +WebrtcGmpVideoEncoder::GetNextNALUnit(const uint8_t **aData,
> > +                                      size_t *aSize,
> > +                                      const uint8_t **aNalStart,
> > +                                      size_t *aNalSize,
> > +                                      bool aStartCodeFollows) // always true?
> 
> Now might be a good time to figure out if aStartCodeFollows is always true.

I just realized that getNextNALUnit() isn't code ekr wrote, it's directly copied from stagefright which is under the Apache license.  This at minimum would require it live in a separate file; or we need to rewrite it or make it moot.

I'm having a separate conversation with Stefan Holmer and Niklas Enbom of Google about the interface to be used for H.264 in Webrtc, and we'd like to use the existing RTPFragmentationHeader interface, which is basically a list of pointers to "naked" NALs and sizes and timestamps, etc.  On the inside of the sandbox, the OpenH264 code would need to do the equivalent to the getNextNALUnit and convert dumps of multiple NALs into a list of buffers and sizes, etc.

This is a better "generic" interface for video codecs, since it gets rid of H264-specific start-code stuff from the API.  (VP8 uses this for example.)

This API change would make this function moot.
 
> @@ +442,5 @@
> > +  // TODO(ekr@rtfm.com): transfer settings from aCodecSettings to codec.
> > +  GMPVideoCodec codec;
> > +  memset(&codec, 0, sizeof(codec));
> > +
> > +  GMPVideoErr err = mGMP->InitDecode(codec, this, 1); // XXX 1?
> 
> The "XXX 1" comment refers to the core count. I'd resolve what you want to
> do here and get rid of the alarming comment.

Originally that was effectively me reviewing the code.
Updated and changed to an OMX-ish format for encoded data.  We'll need to remove GetNextNALUnit, but that will need to be coordinated with the code inside the sandbox, so I temporarily included it to convert to the new format here.
Attachment #8433355 - Attachment is obsolete: true
Attachment #8433355 - Flags: review?(tterribe)
Attached patch interdiffs.patchSplinter Review
Interdiffs for patch update
Attachment #8435108 - Flags: review?(tterribe)
Attachment #8435108 - Flags: review?(joshmoz)
Comment on attachment 8435108 [details] [diff] [review]
Implement GMP codec interface to webrtc

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

r+ with this stuff fixed

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +132,5 @@
> +  mHost = host;
> +
> +  if (!gmp || !host) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }

Do you not want to check this stuff before assigning to mGMP and mHost? I think this happens in InitDecode_g as well.

@@ +177,5 @@
> +int32_t
> +WebrtcGmpVideoEncoder::Encode_g(const webrtc::I420VideoFrame* aInputImage,
> +                                const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
> +                                const std::vector<webrtc::VideoFrameType>* aFrameTypes)
> +{

MOZ_ASSERT(mHost);
MOZ_ASSERT(mGMP);

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.h
@@ +69,5 @@
> +                           uint32_t aFrameRate);
> +
> +  // GMPEncoderCallback virtual functions.
> +  virtual void Encoded(GMPVideoEncodedFrame* aEncodedFrame,
> +                       const GMPCodecSpecificInfo& aCodecSpecificInfo);

Still missing MOZ_OVERRIDE, same for GMPDecoderCallback methods below.
Attachment #8435108 - Flags: review?(joshmoz) → review+
Attachment #8435108 - Attachment is obsolete: true
Attachment #8435108 - Flags: review?(tterribe)
Comment on attachment 8436119 [details] [diff] [review]
Implement GMP codec interface to webrtc

Note: I haven't merged the couple of nit's from josh's last r+
Attachment #8436119 - Flags: review?(ethanhugg)
Attachment #8436119 - Attachment is obsolete: true
Attachment #8436119 - Flags: review?(ethanhugg)
Attachment #8436161 - Flags: review?(ethanhugg)
Comment on attachment 8436161 [details] [diff] [review]
Implement GMP codec interface to webrtc

carry forward r=jesup,josh
Attachment #8436161 - Flags: review+
Comment on attachment 8436161 [details] [diff] [review]
Implement GMP codec interface to webrtc

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

r+ if you fix assert.  I got it to work with my plugin build after doing this change.  I will upload my loopback test file that forces H264 if anyone else wants to try.

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +302,5 @@
> +        if (zeros >= 2) {
> +          // Found start code 0x000001 or 0x00000001
> +          MOZ_ASSERT(zeros == 3); // current temp code only handles 4-byte codes
> +          // now find the length of the NAL
> +          *aData = data++; // start of actual data

I had to change this to *aData = ++data; in order to prevent the assert on line 360.  Before that the difference was always coming back 3 instead of 4.

@@ +330,5 @@
> +        }
> +      }
> +      zeros = 0;
> +    }
> +    data++;

I may be misreading this but I was expecting you to error on something here that is not 0 or 1.  So - 
if (*data == 0) {} else if (*data == 1) {} else { return -1; }.  
That seems to be what the other code did (but it returned -2).
Did you do this on purpose in case there's junk before the zeros start?
Attachment #8436161 - Flags: review?(ethanhugg) → review+
Attached file pc_test_h264.html
Version of pc_test.html which removes VP8 from the offer if H264 is present.  So it uses H264 if you have the OpenH264 GMP plugin loaded.  Modified by Enda.
(In reply to Ethan Hugg [:ehugg] from comment #42)
> Comment on attachment 8436161 [details] [diff] [review]
> Implement GMP codec interface to webrtc
> 
> Review of attachment 8436161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if you fix assert.  I got it to work with my plugin build after doing
> this change.  I will upload my loopback test file that forces H264 if anyone
> else wants to try.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
> @@ +302,5 @@
> > +        if (zeros >= 2) {
> > +          // Found start code 0x000001 or 0x00000001
> > +          MOZ_ASSERT(zeros == 3); // current temp code only handles 4-byte codes
> > +          // now find the length of the NAL
> > +          *aData = data++; // start of actual data
> 
> I had to change this to *aData = ++data; in order to prevent the assert on
> line 360.  Before that the difference was always coming back 3 instead of 4.

Yup, that was what was intended.  My paranoia helped here. :-)  Thanks.

> 
> @@ +330,5 @@
> > +        }
> > +      }
> > +      zeros = 0;
> > +    }
> > +    data++;
> 
> I may be misreading this but I was expecting you to error on something here
> that is not 0 or 1.  So - 
> if (*data == 0) {} else if (*data == 1) {} else { return -1; }.  
> That seems to be what the other code did (but it returned -2).
> Did you do this on purpose in case there's junk before the zeros start?

Yes, that was why.  " // Don't assume we start with a start code (paranoia)".  I could relax that.
This has Windows bustage. I'm about to run out the door, so I'm closing inbound for now. I'll backout when I get home if it's not already taken care of.
Flags: needinfo?(rjesup)
Attachment #8436557 - Flags: review?(pkerr)
Attachment #8436557 - Flags: review?(pkerr) → review+
https://hg.mozilla.org/mozilla-central/rev/a5a8cc3a2908
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.