Closed Bug 911046 Opened 11 years ago Closed 10 years ago

WebRTC OmxCodec coding path support

Categories

(Core :: WebRTC: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.0 S1 (9may)
feature-b2g 2.0

People

(Reporter: u459114, Assigned: jhlin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ft:multimedia-platform][webrtc])

Attachments

(9 files, 28 obsolete files)

1.10 KB, patch
jesup
: review+
ekr
: review+
Details | Diff | Splinter Review
54.54 KB, text/plain
Details
9.85 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
13.85 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
26.20 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
38.74 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
7.61 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
3.81 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
13.00 KB, patch
jhlin
: review+
Details | Diff | Splinter Review
Construct a H.264 coding module and transmit H.264 coded frames over H.264
1. Compare power consumpsion with VP8 codec
2. Compare rendering performance with VP8 codec.
Summary: WebRTC H.264 codec support → WebRTC H.264 OmxCodec support
Summary: WebRTC H.264 OmxCodec support → WebRTC OmxCodec support
1. Redirect coding path to OmxCodec module to using HW component in StageFright
2. Change RTP packetizer to carry differenct coded frame
Summary: WebRTC OmxCodec support → WebRTC OmxCodec coding path support
No longer blocks: b2g-multimedia
Experimental code for supporting H.264 video in WebRTC through libstagefright.
Blocks: 984239
WIP patch to use stagefright MediaCodec API to encode/decode H.264 video.
Attachment #8340183 - Attachment is obsolete: true
- Expose graphic buffers containing decoded frames to MediaCodec output buffers.
- Don't return min Undequeued buffers to ANativeWindow (like bug 911548).
Fix build break on non-B2G platform and update external codec registration machenism.
Attachment #8392643 - Attachment is obsolete: true
Depends on: 985878
Hi Maire,
NI you to get your attention of progress here.

By applying these two WIP patches, WebRTC with H264 works ok on device.
1. Occasional crash: we still met crash because of object reference count, John is working on that.
2. bug 985878: another refcount thread safe problem. We have workaround in WIPs.
3. You can do device-to-device video call, or device-to-desktop video call. For device-to-desktop connection, you need to clone ekr's openh264 repro. Here are the instructions provided by JW:

i. build prerequisite
NASM needed to be installed for assembly code: workable version 2.07 or above, nasm can downloaded from http://www.nasm.us/
On linux: 'sudo apt-get install nasm' to install nasm
On Mac: nasm (v 0.98) bundled in 10.9 is too old. 'brew install nasm' to install newer version and use /usr/local/bin/nasm instead of /usr/bin/nasm

ii. build command (one line bootstrap)
a. git clone https://github.com/jwwang/gecko-dev-openh264.git 
b. cd gecko-dev-openh264
c. git checkout openh264_integration
d. ./config-openh264.sh 
e. ./build-openh264.sh 
f. ./mach build
Flags: needinfo?(mreavy)
Can we get this in shape to land this? Its ok if disabled for now, but we need this code landed so we can start improving it. We have partners who want to help with performance. Whats missing to get this in?
Thanks so much, John and CJ, for the patch and updated report.  

Last week the remaining work was:
1) Code clean up (which John is doing now)
2) video latency when using the hardware codec (which gcp will look at and we'll get the QC involved with if we can't find a quick solution)
3) Implementing a codec chooser or selector (which OpenH264 also needs) -- we said we can use a H.264 pref in the initial landing patch to get around this
4) Use OmxTrackEncoder inside WebRTC

Are you still seeing video latency with John's latest WIP patch?

I don't believe any of the work immediately above or any of the work mentioned in Comment 6 would prevent us from landing this code disabled on trunk.  Are there any new issues and/or work that aren't captured in this bug?

If not, can you propose a simple plan in this bug for landing what we currently have on trunk disabled (behind a pref)?  If you need help with setting up the pref, let me know.

In the meantime, gcp is trying the newest patch to reproduce the results you reported in Comment 6.

Thanks again for your hard work on this!!
Flags: needinfo?(mreavy)
Flags: needinfo?(jolin)
Flags: needinfo?(cku)
(In reply to Maire Reavy [:mreavy] (Please needinfo me if you need my attention on a bug) from comment #8)
> Thanks so much, John and CJ, for the patch and updated report.  
> 
> Last week the remaining work was:
> 1) Code clean up (which John is doing now)
> 2) video latency when using the hardware codec (which gcp will look at and
> we'll get the QC involved with if we can't find a quick solution)
We may figure out this problem after landing.
> 3) Implementing a codec chooser or selector (which OpenH264 also needs) --
> we said we can use a H.264 pref in the initial landing patch to get around
> this
No need. We merge EKR's hack in OpenH264 repo. SDP and codec chooser are involved.
> 4) Use OmxTrackEncoder inside WebRTC
1) + 4) = one week need.
As a result, we will make sure have an review? patch before next Tuesday.
Flags: needinfo?(cku)
Thank you CJ. :)
Flags: needinfo?(jolin)
Comment on attachment 8395583 [details] [diff] [review]
[WIP] WebRTC H.264 codec using stagefright.

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

I went through some of this; some comments below. Please add me as a reviewer when this goes for review.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +887,5 @@
> +                                             pltype,
> +                                             static_cast<
> +                                             WebrtcVideoDecoder*>(decoder));
> +#ifdef WEBRTC_GONK
> +  // FIXME: create I420VideoFrame sublass that carries gralloc images to obsolete

Please file bugs for this and other FIXMEs

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +1,5 @@
> +/* 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 "CSFLog.h"

This file appears to be H.264 specific. Maybe a different name than "Ext" is required. "Omx" perhaps?

@@ +50,5 @@
> +#define EXT_LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, LOG_TAG, __VA_ARGS__)
> +#define EXT_LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
> +#define EXT_LOGI(...) __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__)
> +#define EXT_LOGW(...) __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__)
> +#define EXT_LOGE(...) __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, __VA_ARGS__)

This should use Firefox logging constructs, not android logging constructs.

@@ +669,5 @@
> +
> +int32_t WebrtcExtVideoDecoder::Decode(
> +    const webrtc::EncodedImage& inputImage,
> +    bool missingFrames,
> +    const webrtc::RTPFragmentationHeader* fragmentation,

Note that this still isn't doing correct RTP fragmentation. We need to resolve this
at some point.
(In reply to Eric Rescorla (:ekr) from comment #11)
> Comment on attachment 8395583 [details] [diff] [review]
> [WIP] WebRTC H.264 codec using stagefright.
> 
> Review of attachment 8395583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I went through some of this; some comments below. Please add me as a
> reviewer when this goes for review.
> 
 Sure. Thanks!

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +887,5 @@
> > +                                             pltype,
> > +                                             static_cast<
> > +                                             WebrtcVideoDecoder*>(decoder));
> > +#ifdef WEBRTC_GONK
> > +  // FIXME: create I420VideoFrame sublass that carries gralloc images to obsolete
> 
> Please file bugs for this and other FIXMEs
 Working on cleaning FIXMEs/TODOs now. Will file bugs for remaining ones once my patches are ready for review.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +1,5 @@
> > +/* 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 "CSFLog.h"
> 
> This file appears to be H.264 specific. Maybe a different name than "Ext" is
> required. "Omx" perhaps?
 Yes. I'd like to call it WebrtcOMXAVCCodec. :)

> 
> @@ +50,5 @@
> > +#define EXT_LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, LOG_TAG, __VA_ARGS__)
> > +#define EXT_LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
> > +#define EXT_LOGI(...) __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__)
> > +#define EXT_LOGW(...) __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__)
> > +#define EXT_LOGE(...) __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, __VA_ARGS__)
> 
> This should use Firefox logging constructs, not android logging constructs.
 Okay. Will change them to CSFLog.

> 
> @@ +669,5 @@
> > +
> > +int32_t WebrtcExtVideoDecoder::Decode(
> > +    const webrtc::EncodedImage& inputImage,
> > +    bool missingFrames,
> > +    const webrtc::RTPFragmentationHeader* fragmentation,
> 
> Note that this still isn't doing correct RTP fragmentation. We need to
> resolve this
> at some point.
 Have to confess that I don't even realize that fragmentation matters. :) Could you please help provide some info/doc/example describing how it works?
See RFC 6184; in particular single-nal (mode 0) and non-interleaved fragmented (mode 1) support.  (Mode 2 is for non-realtime media.)  Packetization in mode 0 is fairly simple, but also means the encoder must be informed of the MTU size.  Mode 1 allows the encoder to generate more efficient encoding and also merge NALs to reduce packet counts for stuff like in-band parameter sets for IDRs (i-frames) - but it also means that a lost packet means the entire frame is lost; you can't decode part of a frame.  In practice I don't generally see that as a problem and personally prefer using mode 1; but mode 1 is the default.

The tricky part is that if the other side only offers mode 1, you MUST do mode 1.  And an fmtp line can only have one mode specified; if you want to offer mode 0 and mode 1, you must use two payload types and see which is accepted (both can get fun...)
Depends on: 988713
Introduce external codec related interfaces to video conduit.
The change to scoped_ptr.h is needed because macro (FF) name collision between it and nsCRTGlue.h
Attachment #8392645 - Attachment is obsolete: true
Attachment #8399296 - Flags: review?(rjesup)
Attachment #8399296 - Flags: review?(ekr)
Use webrtc::TextureVideoFrame to carry gralloc-ed decoded frames through WebRTC rendering pipeline.
Attachment #8399298 - Flags: review?(rjesup)
Attachment #8399298 - Flags: review?(ekr)
Make OMXVideoEncoder output blobs in NAL format to support WebRTC.
Attachment #8399299 - Flags: review?(roc)
Create external codec factory and HW H.264 codec implementation for B2G.
Encoder uses existing stagefright wrapper class developed for Media recording API. Also provides a simple decoder that calls stagefright API directly until B2G platform decoder module (bug 984223) is ready.
Attachment #8399306 - Flags: review?(rjesup)
Attachment #8399306 - Flags: review?(ekr)
Use 'I420' codec config to cheat and register external codec.
Attachment #8399308 - Flags: review?(rjesup)
Attachment #8399308 - Flags: review?(ekr)
Use preference value 'media.peerconnection.video.h264_enabled' to enable/disable H.264 support on B2G.
Attachment #8399309 - Flags: review?(rjesup)
Attachment #8399309 - Flags: review?(ekr)
Attachment #8399326 - Flags: review?(rjesup)
Attachment #8399326 - Flags: review?(ekr)
Blocks: 989944
Blocks: 989945
Comment on attachment 8399299 [details] [diff] [review]
Part 3: Add support to WebRTC in OMXCodecWrapper.

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

::: content/media/omx/OMXCodecDescriptorUtil.cpp
@@ +240,5 @@
>    nsTArray<AVCParamSet> mPPS;
>  };
>  
> +static
> +size_t

"static size_t" on the same line

@@ +261,5 @@
> +}
> +
> +// Convert decoder config descriptor blob to SPS + PPS blob.
> +static
> +void

"static void" on the same line

::: content/media/omx/OMXCodecDescriptorUtil.h
@@ +21,3 @@
>  status_t GenerateAVCDescriptorBlob(ABuffer* aData,
> +                                   nsTArray<uint8_t>* aOutputBuf,
> +                                   bool aNAL);

Make this an enum with two values instead of a bool

::: content/media/omx/OMXCodecWrapper.h
@@ +259,5 @@
>    nsresult Encode(const mozilla::layers::Image* aImage, int aWidth, int aHeight,
>                    int64_t aTimestamp, int aInputFlags = 0);
>  
> +  /** Set encoding bitrate (in kbps). */
> +  nsresult SetBitrate(int32_t aBitrate);

Name this aKbps instead

@@ +292,5 @@
>    friend class OMXCodecWrapper;
>  
>    int mWidth;
>    int mHeight;
> +  bool mNAL;

Use the new enum here
Attachment #8399299 - Flags: review?(roc) → review-
Comment on attachment 8399296 [details] [diff] [review]
Part 1: Support external codec in VideoConduit.

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

Some small changes needed.   If we can avoid changing scoped_ptr.h that's a plus.

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

Remove, and add an mPtrExtCodec = nullptr before the engine is released (see the other scoped ptrs)

@@ +283,5 @@
>      return kMediaConduitSessionNotInited;
>    }
>  
> +  mPtrExtCodec = webrtc::ViEExternalCodec::GetInterface(mVideoEngine);
> +  if (!mPtrExtCodec) {

use the same pattern as the other GetInterface calls "if( !(mPtrExCodec = ...."

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +213,5 @@
>                        mVideoEngine(nullptr),
>                        mTransport(nullptr),
>                        mRenderer(nullptr),
>                        mPtrExtCapture(nullptr),
> +                      mPtrExtCodec(nullptr),

remove

@@ +294,5 @@
>    ScopedCustomReleasePtr<webrtc::ViERender> mPtrViERender;
>    ScopedCustomReleasePtr<webrtc::ViERTP_RTCP> mPtrRTP;
>  
>    webrtc::ViEExternalCapture* mPtrExtCapture; // shared
> +  webrtc::ViEExternalCodec*  mPtrExtCodec;

ScopedCustomReleasePtr

::: media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h
@@ +29,3 @@
>  #include <stdlib.h>            // for free() decl
>  
> +#include <cstddef>             // for std::ptrdiff_t

Why is this change needed?

@@ +187,5 @@
>  
>  // scoped_ptr_malloc<> is similar to scoped_ptr<>, but it accepts a
>  // second template argument, the function used to free the object.
>  
> +template<typename T, void (*FFF)(void*) = free> class scoped_ptr_malloc {

Please change it to something even less likely to collide with #defines, like SCOPED_PTR_FREE_FUNC or some such.

Note also this is an upstream file, so any changes here will need to be upstreamed to webrtc.org.


Or do what is done elsewhere - before including things that include scoped_ptr.h, #ifdef FF #undef FF #endif.
Attachment #8399296 - Flags: review?(rjesup) → review-
Comment on attachment 8399298 [details] [diff] [review]
Part 2: Support 'handle-using' video frames for WebRTC on B2G.

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

r+ with switch from void * to opaque pointer type

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +186,4 @@
>     */
> +  virtual bool IsTextureSupported() {
> +#ifdef WEBRTC_GONK
> +    return true;

Is this dependent on whether we're using the OMX decoder?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +1418,5 @@
>    ReentrantMonitorAutoEnter enter(monitor_);
>  
> +  if (buffer) {
> +    // Create a video frame using |buffer| and append it to the track.
> +    nsRefPtr<layers::Image> image = image_container_->CreateImage(ImageFormat::PLANAR_YCBCR);

We're never going to use GRALLOC_PLANAR_YCBCR now?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +643,5 @@
>      virtual void RenderVideoFrame(const unsigned char* buffer,
>                                    unsigned int buffer_size,
>                                    uint32_t time_stamp,
> +                                  int64_t render_time,
> +                                  void* handle) {

Can we use an opaque pointer type instead of "void *"?

@@ +648,2 @@
>        pipeline_->listener_->RenderVideoFrame(buffer, buffer_size, time_stamp,
> +                                            render_time, handle);

since you're touching this line, add a space to align
Attachment #8399298 - Flags: review?(rjesup) → review+
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +77,5 @@
> +
> +  status_t Start()
> +  {
> +    status_t err = mCodec->start();
> +    mStarted = err == OK;

(err == OK)
If it's not ok, should we still getInputBuffers/getOutputBuffers?
perhaps MOZ_ASSERT(!mStarted) at the top

@@ +88,5 @@
> +
> +  status_t Stop()
> +  {
> +    status_t err = mCodec->stop();
> +    mStarted = err == OK;

Shouldn't that be !(err == OK)?  Or even better perhaps:
if (mStarted) {
  err = mCodec->stop();
  if (err == OK) {
    mStarted = false;
  }
}

@@ +149,5 @@
> +
> +private:
> +  sp<MediaCodec> mOmx;
> +  uint32_t mBufferIndex;
> +  Monitor mMonitor;

Document what's protected by the Monitor

@@ +171,5 @@
> +    int cnt = ++mRefCnt;
> +    return cnt;
> +  }
> +
> +  int Release()

Why are we hand-rolling AddRef and Release here?

@@ +174,5 @@
> +
> +  int Release()
> +  {
> +    MOZ_ASSERT(mRefCnt > 0);
> +    void* that = this;

What is this meant to do?  'that' isn't used again

@@ +175,5 @@
> +  int Release()
> +  {
> +    MOZ_ASSERT(mRefCnt > 0);
> +    void* that = this;
> +    void* handle = mImage.get();

what does this do?  handle isn't used again in Release()

@@ +195,5 @@
> +  , mMutex("WebrtcOMXH264VideoEncoder")
> +  , mOMX(nullptr)
> +  , mOMXConfigured(false)
> +{
> +  memset(&mEncodedImage, 0, sizeof(mEncodedImage));

This is a class, you can't just set it's memory to 0 safely.  In this case, the constructor initializes everything needed.  Remove.

@@ +207,5 @@
> +  AMessage* format = new AMessage;
> +
> +  format->setString("mime", MEDIA_MIMETYPE_VIDEO_AVC);
> +  format->setInt32("store-metadata-in-buffers", 0);
> +  format->setInt32("prepend-sps-pps-to-idr-frames", 0);

Are we using sprop-parameter-sets in the SDP?  If not, we need to insert SPS/PPS NALUs

@@ +208,5 @@
> +
> +  format->setString("mime", MEDIA_MIMETYPE_VIDEO_AVC);
> +  format->setInt32("store-metadata-in-buffers", 0);
> +  format->setInt32("prepend-sps-pps-to-idr-frames", 0);
> +  format->setInt32("bitrate", codecSettings->minBitrate * 1000); // kbps->bps

Likely the codec shouldn't start at the minimum-allowed bitrate

@@ +213,5 @@
> +  format->setInt32("width", codecSettings->width);
> +  format->setInt32("height", codecSettings->height);
> +  format->setInt32("stride", codecSettings->width);
> +  format->setInt32("slice-height", codecSettings->height);
> +  format->setFloat("frame-rate", (float)codecSettings->maxFramerate);

What assumptions does the OMX encoder draw from this setting?

@@ +219,5 @@
> +  // <B2G>/hardware/qcom/media/mm-video/vidc/venc/src/video_encoder_device.cpp:
> +  // venc_dev::venc_set_color_format()
> +  format->setInt32("color-format", OMX_COLOR_FormatYUV420SemiPlanar);
> +  // FIXME: get correct parameters from codecSettings?
> +  format->setInt32("i-frame-interval", 1); // one I-frame per sec

We should never send an iframe except as a result of packet loss or at the start of a stream (unless the encoder decides it's cheaper, like in a scene change perhaps)

@@ +222,5 @@
> +  // FIXME: get correct parameters from codecSettings?
> +  format->setInt32("i-frame-interval", 1); // one I-frame per sec
> +  format->setInt32("profile", OMX_VIDEO_AVCProfileBaseline);
> +  format->setInt32("level", OMX_VIDEO_AVCLevel3);
> +  //format->setInt32("bitrate-mode", OMX_Video_ControlRateConstant);

What would this do if enabled?  Please comment why it's not

@@ +246,5 @@
> +  MutexAutoLock lock(mMutex);
> +
> +  // TODO: eliminate extra pixel copy & color conversion
> +  size_t size = codecSettings->width * codecSettings->height * 3 / 2;
> +  if (mEncodedImage._size < size) {

An encoded image can be larger than the source image.  This should be a null pointer initially, and then a high water mark (or allocate a "reasonable" buffer to start, and realloc larger if needed).  See the mods made in response to reviews in bug 969395.

In fact, this code is *remarkably* similar to that code (before I had Qi modify it).  Is this cut-and-pasted from examples?  Or was that patch based on an earlier version of this one, or vice-versa?

@@ +264,5 @@
> +    const webrtc::CodecSpecificInfo* codecSpecificInfo,
> +    const std::vector<webrtc::VideoFrameType>* frame_types)
> +{
> +  CODEC_LOGD("this:%p will encode", this);
> +  if (mOMX == nullptr) {

if this is a raw ptr, "if (mOMX) {".  If it's an sp pointer, then the explicit == nullptr is likely needed.

@@ +288,5 @@
> +  MOZ_ASSERT(inputImage.stride(webrtc::kUPlane) == inputImage.stride(webrtc::kVPlane));
> +  yuvData.mCbCrStride = inputImage.stride(webrtc::kUPlane);
> +  yuvData.mCbChannel = const_cast<uint8_t*>(inputImage.buffer(webrtc::kUPlane));
> +  yuvData.mCrChannel = const_cast<uint8_t*>(inputImage.buffer(webrtc::kVPlane));
> +  yuvData.mCbCrSize = gfx::IntSize(yuvData.mYSize.width / 2, yuvData.mYSize.height / 2);

width and height can be odd; add 1 before dividing by 2

@@ +318,5 @@
> +
> +  EncodedFrame frame;
> +  frame.mWidth = inputImage.width();
> +  frame.mHeight = inputImage.height();
> +  frame.mTimestamp = inputImage.timestamp();

What point is "EncodedFrame frame;" having here?  Nothing seems to care about it later.

@@ +335,5 @@
> +
> +  MutexAutoLock lock(mMutex);
> +
> +  mEncodedImage._length = output.Length();
> +  memcpy(mEncodedImage._buffer, output.Elements(), mEncodedImage._length);

Add a comment here about working to avoid memcpy's in the future.  See VP8 bug

@@ +339,5 @@
> +  memcpy(mEncodedImage._buffer, output.Elements(), mEncodedImage._length);
> +  mEncodedImage._completeFrame = true;
> +  mEncodedImage._frameType =
> +    (flags & (MediaCodec::BUFFER_FLAG_SYNCFRAME | MediaCodec::BUFFER_FLAG_CODECCONFIG))?
> +    webrtc::kKeyFrame:webrtc::kDeltaFrame;

spaces around ':'

@@ +362,5 @@
> +{
> +  CODEC_LOGD("this:%p will be released", this);
> +  if (mOMX) {
> +    delete mOMX;
> +    mOMX = nullptr;

Again, see if we can use a reference-counted type here and avoid explicit release

@@ +385,5 @@
> +// TODO
> +int32_t
> +WebrtcOMXH264VideoEncoder::SetChannelParameters(uint32_t packetLoss, int rtt)
> +{
> +  return WEBRTC_VIDEO_CODEC_OK;

Add more than just // TODO.  Please file a bug and include the # here, and describe what the impact of not implementing this now is.

@@ +436,5 @@
> +  static int64_t firstTime = -1ll;
> +  size_t index;
> +  uint32_t time = PR_IntervalNow();
> +  status_t err = omx->dequeueInputBuffer(&index,
> +    firstTime < 0 ? START_DEQUEUE_BUFFER_TIMEOUT_US : DEQUEUE_BUFFER_TIMEOUT_US);

parens around "firstTime < 0" for consistency with below

@@ +447,5 @@
> +  time = PR_IntervalNow();
> +
> +  uint32_t flags = 0;
> +  if (inputImage._frameType == webrtc::kKeyFrame) {
> +    flags |= (firstTime < 0)? MediaCodec::BUFFER_FLAG_CODECCONFIG:MediaCodec::BUFFER_FLAG_SYNCFRAME;

flags =
flags is 0 before this line
spaces around ':'

@@ +457,5 @@
> +  memcpy(omxIn->data(), inputImage._buffer, size);
> +  if (firstTime < 0) {
> +    firstTime = inputImage._timeStamp;
> +  }
> +  *timeUs = (inputImage._timeStamp - firstTime) * 10; // FIXME: use time of input image.

timestamp rate (if it's from RTP) is 90000/sec, not 100000

@@ +592,5 @@
> +  if (inputImage._length== 0 || !inputImage._buffer) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  uint32_t time = PR_IntervalNow();

All the debugs for timing should be #ifdef DEBUG or perhaps #ifdef TIME_H264_OMX

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.h
@@ +9,5 @@
> + *  Use of this source code is governed by a BSD-style license
> + *  that can be found in the LICENSE file in the root of the source
> + *  tree. An additional intellectual property rights grant can be found
> + *  in the file PATENTS.  All contributing project authors may
> + *  be found in the AUTHORS file in the root of the source tree.

So usually we try to have one license per file, I believe.  Please ping gerv as to what if anything is needed here

@@ +61,5 @@
> +
> +  // Implement VideoEncoder interface.
> +  virtual int32_t InitEncode(const webrtc::VideoCodec* codecSettings,
> +                                   int32_t numberOfCores,
> +                                   uint32_t maxPayloadSize);

indent is wrong

@@ +76,5 @@
> +  virtual int32_t SetChannelParameters(uint32_t packetLoss,
> +                                             int rtt);
> +
> +  virtual int32_t SetRates(uint32_t newBitRate,
> +                                 uint32_t frameRate);

several more wrong indents.  Pick one style and stick to it.

@@ +81,5 @@
> +
> + private:
> +  android::OMXVideoEncoder* mOMX;
> +  webrtc::EncodedImageCallback* mCallback;
> +  Mutex mMutex;

Document here what vars are protected by the mMutex, and delineate them by grouping them in the source to make it clear

@@ +113,5 @@
> +
> +  virtual int32_t Reset();
> +
> + private:
> +  WebrtcOMX* mOMX;

Since WebrtcOMX supports refcounting, why is it being stored here as a raw pointer?
Attachment #8399306 - Flags: review?(rjesup) → review-
Comment on attachment 8399308 [details] [diff] [review]
Part 5: Register H.264 external codec for B2G.

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

As a temporary hack to test this fine, but not for landing (unless very temporary and something else is tracking fixing it)

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2122,5 @@
> +{
> +  MediaConduitErrorCode err = kMediaConduitNoError;
> +#ifdef WEBRTC_GONK
> +  // Here we use "I420" to register H.264 because WebRTC.org code has a
> +  // whitelist of supported video codec in |webrtc::ViECodecImpl::CodecValid()|

So we modify CodecValid() instead, or add an interface to add values to CodecValid()
Attachment #8399308 - Flags: review?(rjesup) → review-
Comment on attachment 8399309 [details] [diff] [review]
Part 6: Make H.264 preferred video codec when enabled in preferences.

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

r+ with changes to match whatever is done in the previous patch to avoid replacing I420 with H.264

::: media/webrtc/signaling/src/sipcc/core/common/prot_configmgr.c
@@ +632,5 @@
>        count++;
>      }
> +    if ( codec_mask & VCM_CODEC_RESOURCE_VP8) {
> +      aSupportedCodecs[count] = RTP_VP8;
> +      count++;

I should note that his enforces H.264 is preferred to VP8.  To resolve this, codec_mask probably needs to be rethought.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4665,5 @@
>           * Mask out the types that we do not support
>           */
>          switch (codec) {
>              case RTP_VP8:
> +            case RTP_I420:

RTP_H264 (and per previous patch, add it if needed)
Attachment #8399309 - Flags: review?(rjesup) → review+
Attachment #8399326 - Flags: review?(rjesup) → review+
Comment on attachment 8399296 [details] [diff] [review]
Part 1: Support external codec in VideoConduit.

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +38,5 @@
>  class WebrtcAudioConduit;
>  
> +// Interface of external video encoder for WebRTC.
> +class WebrtcVideoEncoder
> +  : public VideoEncoder

Please match the eindents in the rest of this file.

Also, style here is to use "webrtc::"

@@ +145,5 @@
> +   * Set an external encoder object |encoder| to the payload type |pltype|
> +   * for sender side codec.
> +   */
> +  virtual MediaConduitErrorCode SetExternalSendCodec(int pltype,
> +                VideoEncoder* encoder);

Match indent

@@ +152,5 @@
> +   * Set an external decoder object |decoder| to the payload type |pltype|
> +   * for receiver side codec.
> +   */
> +  virtual MediaConduitErrorCode SetExternalRecvCodec(int pltype,
> +                VideoDecoder* decoder);

Same as above.

@@ +294,5 @@
>    ScopedCustomReleasePtr<webrtc::ViERender> mPtrViERender;
>    ScopedCustomReleasePtr<webrtc::ViERTP_RTCP> mPtrRTP;
>  
>    webrtc::ViEExternalCapture* mPtrExtCapture; // shared
> +  webrtc::ViEExternalCodec*  mPtrExtCodec;

Why isn't this a ScopedCustomReleaaePtr

::: media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h
@@ +29,4 @@
>  #include <stdlib.h>            // for free() decl
>  
> +#include <cstddef>             // for std::ptrdiff_t
> +

These are changes to upstream code. Is there a reason why you need to change this?

@@ +148,5 @@
>        delete [] arr;
>      }
>    }
>  
> +  T& operator[](std::ptrdiff_t i) const {

can't you just do using std in any code that includes this?

@@ +187,5 @@
>  
>  // scoped_ptr_malloc<> is similar to scoped_ptr<>, but it accepts a
>  // second template argument, the function used to free the object.
>  
> +template<typename T, void (*FFF)(void*) = free> class scoped_ptr_malloc {

Yeah, this is a real problem. Are you going to upstream it?
Attachment #8399296 - Flags: review?(ekr)
Comment on attachment 8395583 [details] [diff] [review]
[WIP] WebRTC H.264 codec using stagefright.

By latest patches.
Attachment #8395583 - Attachment is obsolete: true
Attachment #8392645 - Attachment is obsolete: false
Attachment #8392645 - Attachment description: [WIP] Changes to stagefright to make attachment 8392643 work. → [WIP] Changes to stagefright to make other patches work.
(In reply to Eric Rescorla (:ekr) from comment #27)
> Comment on attachment 8399296 [details] [diff] [review]
> Part 1: Support external codec in VideoConduit.
> 
> Review of attachment 8399296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +38,5 @@
> >  class WebrtcAudioConduit;
> >  
> > +// Interface of external video encoder for WebRTC.
> > +class WebrtcVideoEncoder
> > +  : public VideoEncoder
> 
> Please match the eindents in the rest of this file.
> 
> Also, style here is to use "webrtc::"

 What do you mean by using "webrtc::" style? Could you please give an example?
(In reply to Randell Jesup [:jesup] from comment #23)
> Comment on attachment 8399298 [details] [diff] [review]
> Part 2: Support 'handle-using' video frames for WebRTC on B2G.
> 
> Review of attachment 8399298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with switch from void * to opaque pointer type
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +643,5 @@
> >      virtual void RenderVideoFrame(const unsigned char* buffer,
> >                                    unsigned int buffer_size,
> >                                    uint32_t time_stamp,
> > +                                  int64_t render_time,
> > +                                  void* handle) {
> 
> Can we use an opaque pointer type instead of "void *"?

 Do you mean like |webrtc::NativeHandle*|?
Flags: needinfo?(rjesup)
r- comments addressed:
- avoid name collision error without touching upstream file.
- use ScopedCustomReleasePtr for ext codec interface.
- code style format.
Attachment #8399296 - Attachment is obsolete: true
Attachment #8400373 - Flags: review?(rjesup)
r- comments addressed:
- use enum to indicate output blob format
- rename parameter: aBitrate -> aKbps 
- code style format
Attachment #8399299 - Attachment is obsolete: true
Attachment #8400384 - Flags: review?(roc)
(In reply to John Lin[:jolin][:jhlin] from comment #30)
> (In reply to Randell Jesup [:jesup] from comment #23)
> > Comment on attachment 8399298 [details] [diff] [review]
> > Part 2: Support 'handle-using' video frames for WebRTC on B2G.
> > 
> > Review of attachment 8399298 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r+ with switch from void * to opaque pointer type
> > 
> > ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> > @@ +643,5 @@
> > >      virtual void RenderVideoFrame(const unsigned char* buffer,
> > >                                    unsigned int buffer_size,
> > >                                    uint32_t time_stamp,
> > > +                                  int64_t render_time,
> > > +                                  void* handle) {
> > 
> > Can we use an opaque pointer type instead of "void *"?
> 
>  Do you mean like |webrtc::NativeHandle*|?

No, not really - I meant to define a pointer-type to an undefined structure (even if it's just something like "typedef void *H264CallbackHandle;")  It helps document what you're doing and avoid mixing things by accident.
Flags: needinfo?(rjesup)
Comment on attachment 8400373 [details] [diff] [review]
Part 1: Support external codec in VideoConduit. v1.1

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

r+ with minor style nits

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ +262,5 @@
> +   * @param encoder
> +   * @result: on success, we will use the specified encoder
> +   */
> +  virtual MediaConduitErrorCode SetExternalSendCodec(int pltype,
> +                                VideoEncoder* encoder) = 0;

align with 'int'

@@ +270,5 @@
> +   * @param decoder
> +   * @result: on success, we will use the specified decoder
> +   */
> +  virtual MediaConduitErrorCode SetExternalRecvCodec(int pltype,
> +                                VideoDecoder* decoder) = 0;

ditto

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +868,5 @@
> +                                         VideoDecoder* decoder) {
> +  mPtrExtCodec->RegisterExternalReceiveCodec(mChannel,
> +                                             pltype,
> +                                             static_cast<
> +                                             WebrtcVideoDecoder*>(decoder));

don't break after < -- I'd rather violate length and make it readable.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +42,5 @@
>  
> +// Interface of external video encoder for WebRTC.
> +class WebrtcVideoEncoder
> +  : public VideoEncoder
> +  , public webrtc::VideoEncoder

Minor nit:
Match styles (see following functions):

class WebrtcVideoEncoder:public VideoEncoder
                         ,public webrtc::VideoEncoder

@@ +48,5 @@
> +
> +// Interface of external video decoder for WebRTC.
> +class WebrtcVideoDecoder
> +  : public VideoDecoder
> +  , public webrtc::VideoDecoder

ditto

@@ +148,5 @@
> +   * Set an external encoder object |encoder| to the payload type |pltype|
> +   * for sender side codec.
> +   */
> +  virtual MediaConduitErrorCode SetExternalSendCodec(int pltype,
> +                VideoEncoder* encoder);

Match file - indent second line to match up with 'i' of 'int'

@@ +155,5 @@
> +   * Set an external decoder object |decoder| to the payload type |pltype|
> +   * for receiver side codec.
> +   */
> +  virtual MediaConduitErrorCode SetExternalRecvCodec(int pltype,
> +                VideoDecoder* decoder);

ditto
Attachment #8400373 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #23)
> Comment on attachment 8399298 [details] [diff] [review]
> Part 2: Support 'handle-using' video frames for WebRTC on B2G.
> 
> Review of attachment 8399298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with switch from void * to opaque pointer type
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +186,4 @@
> >     */
> > +  virtual bool IsTextureSupported() {
> > +#ifdef WEBRTC_GONK
> > +    return true;
> 
> Is this dependent on whether we're using the OMX decoder?

 Since IsTextureSupported() is only relevant when video frame uses handle (http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_renderer.cc#172) and SW decoder won't generate handle-using frame, I think this condition is sufficient enough.

> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +1418,5 @@
> >    ReentrantMonitorAutoEnter enter(monitor_);
> >  
> > +  if (buffer) {
> > +    // Create a video frame using |buffer| and append it to the track.
> > +    nsRefPtr<layers::Image> image = image_container_->CreateImage(ImageFormat::PLANAR_YCBCR);
> 
> We're never going to use GRALLOC_PLANAR_YCBCR now?

 Originally I thought using GRALLOC_PLANAR_YCBCR will cost extra memcpy() (from |buffer| to graphic buffer) calls. But it seems video playback code (http://dxr.mozilla.org/mozilla-central/source/content/media/MediaData.cpp#260) believe that's a good idea. Will bring that back in next patch.
Changes to gfx code (bug 985772) breaks the patches. While next version patches are still in work, please check my github: https://github.com/jhlin/gecko-dev/tree/encode-fix-with-timing-code for WIP patches that address the problem if you want to test it. Sorry for the trouble.
Comment on attachment 8399308 [details] [diff] [review]
Part 5: Register H.264 external codec for B2G.

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2122,5 @@
> +{
> +  MediaConduitErrorCode err = kMediaConduitNoError;
> +#ifdef WEBRTC_GONK
> +  // Here we use "I420" to register H.264 because WebRTC.org code has a
> +  // whitelist of supported video codec in |webrtc::ViECodecImpl::CodecValid()|

I suggest that we land this as-is and mark it as a bug and then talk to Justin about the best way to deal with this.
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +46,5 @@
> +
> +// Link to Stagefright
> +class WebrtcOMX MOZ_FINAL
> +  : public AtomicRefCounted<WebrtcOMX>
> +  , public GonkNativeWindowNewFrameCallback

Rename to WebrtcOMXDecoder and MOZ_FINAL

@@ +51,5 @@
> +{
> +public:
> +  MOZ_DECLARE_REFCOUNTED_TYPENAME(WebrtcOMX)
> +
> +  WebrtcOMX(const char* mime, bool encoder)

Remove encoder

@@ +57,5 @@
> +    android::ProcessState::self()->startThreadPool();
> +
> +    mLooper = new ALooper;
> +    mLooper->start();
> +    mCodec = MediaCodec::CreateByType(mLooper, mime, encoder);

create mCodec om Start() function?

 status_t Start()
 {
   if (!mStarted) 
   {
     MOZ_ASSERT(mCodec == nullptr);
     mCodec = MediaCodec::CreateByType(mLooper, mime, encoder);
     ...

And, do we still need mStarted data member after doing this? There is a dependency between mCodec and mStarted --If mCodec has value, it means we have start encode; otherwise, it's not in start state.

@@ +63,5 @@
> +
> +  virtual ~WebrtcOMX()
> +  {
> +    mCodec->release();
> +    mCodec.clear();

Call stop here?

@@ +104,5 @@
> +  sp<ALooper> mLooper;
> +  sp<MediaCodec> mCodec; // OMXCodec
> +  sp<AMessage> mConfig;
> +  int mWidth;
> +  int mHeight;

Do we really need these two variables?

@@ +279,5 @@
> +      PR_IntervalToMilliseconds(PR_IntervalNow()-time));
> +    time = PR_IntervalNow();
> +  }
> +
> +  layers::PlanarYCbCrImage* img = new layers::PlanarYCbCrImage(nullptr);

Add comment here
// Convert I420VideoFrame to PlanarYCbCrData.

Or move code here into a helper function, named as I420VideoFrameToPlanarYCbCrData maybe

@@ +292,5 @@
> +  yuvData.mCbCrSize = gfx::IntSize(yuvData.mYSize.width / 2, yuvData.mYSize.height / 2);
> +  yuvData.mPicSize = yuvData.mYSize;
> +  yuvData.mStereoMode = StereoMode::MONO;
> +  img->SetDataNoCopy(yuvData);
> +

Move #283 to #295

@@ +333,5 @@
> +  CODEC_LOGD("this:%p encode dequeue OMX output buffer len:%u time:%lld flags:0x%08x took %u ms",
> +             this, output.Length(), timeUs, flags, PR_IntervalToMilliseconds(PR_IntervalNow()-time));
> +
> +  MutexAutoLock lock(mMutex);
> +

Do we need check mEncoderImage._size here? Is that possible that WebrtcOMXH264VideoEncoder::Encode be called after WebrtcOMXH264VideoEncoder::Release()?

@@ +334,5 @@
> +             this, output.Length(), timeUs, flags, PR_IntervalToMilliseconds(PR_IntervalNow()-time));
> +
> +  MutexAutoLock lock(mMutex);
> +
> +  mEncodedImage._length = output.Length();

MOZ_ASSERT(mEncodedImage._length < mEncodedImage._size);

@@ +342,5 @@
> +    (flags & (MediaCodec::BUFFER_FLAG_SYNCFRAME | MediaCodec::BUFFER_FLAG_CODECCONFIG))?
> +    webrtc::kKeyFrame:webrtc::kDeltaFrame;
> +
> +  CODEC_LOGD("this:%p encode frame type:%d size:%u", this, mEncodedImage._frameType, mEncodedImage._length);
> +  mCallback->Encoded(mEncodedImage, nullptr, nullptr);

mCallback might be null if RegisterEncodeCompleteCallback not been called

@@ +352,5 @@
> +WebrtcOMXH264VideoEncoder::RegisterEncodeCompleteCallback(
> +    webrtc::EncodedImageCallback* callback)
> +{
> +  mCallback = callback;
> +

MOZ_ASSERT(callback) or null validate before assignment

@@ +420,5 @@
> +    mOMX->mConfig = VideoCodecSettings2AMessage(codecSettings);
> +    mOMX->mNativeWindow = new GonkNativeWindow();
> +    mOMX->mNativeWindow->setNewFrameCallback(mOMX);
> +    mOMX->mNativeWindowClient =
> +        new GonkNativeWindowClient(mOMX->mNativeWindow->getBufferQueue());

Move 420 ~ 424 into WebrtcOMXEncoder::Init function.
Even if we are going to refactory this part in Bug 984223, we should still do initial task in correct place.

Code here should be very simple
 mOMX = new WebrtcOMXDecoder(MEDIA_MIMETYPE_VIDEO_AVC);
 mOMX->Initiate(codecSettings);

@@ +423,5 @@
> +    mOMX->mNativeWindowClient =
> +        new GonkNativeWindowClient(mOMX->mNativeWindow->getBufferQueue());
> +  }
> +  mOMX->mStarted = false;
> +

Remove this line

@@ +432,5 @@
> +status_t
> +feedOMXInput(WebrtcOMX* decoder, const sp<MediaCodec>& omx,
> +             const webrtc::EncodedImage& inputImage, int64_t* timeUs)
> +{
> +  static int64_t firstTime = -1ll;

Are you should we want a static local variable here?
Will this variable back to -1ll in the second video session?

@@ +568,5 @@
> +      decoder->mNativeWindow.get(), frame, omx, index, omxOut, renderTimeMs);
> +    if (!video_frame) {
> +      omx->releaseOutputBuffer(index);
> +    }
> +    callback->Decoded(*video_frame);

callback might be null

@@ +574,5 @@
> +    // OMX buffer will be released after renderer is done with it.
> +  }
> +
> +end:
> +  delete frame;

if (frame) {
  delete frame;
  frame = nullptr;
}

@@ +610,5 @@
> +    mOMX->mConfig->setInt32("stride", width);
> +    mOMX->mConfig->setInt32("slice-height", height);
> +    mOMX->mWidth = width;
> +    mOMX->mHeight = height;
> +    sp<Surface> nativeWindow = nullptr;

Can we move #598~#614 into mOMX->Configure?
If I am a maintainer of WebrtcOMXH264VideoDecoder, I really don't want to know how to setup mConfig on mOMX

@@ +642,5 @@
> +WebrtcOMXH264VideoDecoder::RegisterDecodeCompleteCallback(
> +    webrtc::DecodedImageCallback* callback)
> +{
> +  mCallback = callback;
> +

Validate callback

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.h
@@ +26,5 @@
> +#include "mozilla/Mutex.h"
> +
> +#include "MediaConduitInterface.h"
> +#include "AudioConduit.h"
> +#include "VideoConduit.h"

Do we really include these three headers here?

@@ +41,5 @@
> +struct EncodeConfig
> +{
> +uint32_t mWidth;
> +uint32_t mHeight;
> +uint32_t mFrameRate;

indent

@@ +86,5 @@
> +  std::queue<EncodedFrame> mFrames;
> +  webrtc::EncodedImage mEncodedImage;
> +  size_t mMaxPayloadSize;
> +  bool mOMXConfigured;
> +  struct EncodeConfig mConfig;

remove struct keyword here
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +531,5 @@
> +  EncodedFrame* frame = new EncodedFrame();
> +  frame->mWidth = decoder->mWidth;
> +  frame->mHeight = decoder->mHeight;
> +  frame->mTimestamp = inputImage._timeStamp;
> +

allocate and setup frame in the else statement at #566

@@ +625,5 @@
> +  bool feedFrame = true;
> +
> +  while (feedFrame) {
> +    int64_t timeUs;
> +    status_t err = feedOMXInput(mOMX, omx, inputImage, &timeUs);

feedOMXInput(mOMX, inputImage, &timeUs) -- get omx in feedOMXInput and getOMXOutput

@@ +651,5 @@
> +WebrtcOMXH264VideoDecoder::Release()
> +{
> +  CODEC_LOGD("this:%p will be released", this);
> +
> +  if (mOMX) {

Call Stop before deletion?
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +232,5 @@
> +WebrtcOMXH264VideoEncoder::InitEncode(const webrtc::VideoCodec* codecSettings,
> +                                      int32_t numberOfCores,
> +                                      uint32_t maxPayloadSize)
> +{
> +  mMaxPayloadSize = maxPayloadSize;

Why we need mMaxPayloadSize? And please init mMaxPayloadSize in constructor if this member is required.

@@ +272,5 @@
> +  uint32_t time = PR_IntervalNow();
> +
> +  if (!mOMXConfigured) {
> +    mOMX->Configure(mConfig.mWidth, mConfig.mHeight,
> +                    mConfig.mFrameRate, true /* NAL */);

I am a little bit confuse here.
mConfig is setup in InitEncoder, and then mOMX->Configure be called in WebrtcOMXH264VideoEncoder::Encode.

Why not just call mOMX->Configure in InitEncoder?

@@ +586,5 @@
> +                                  const webrtc::RTPFragmentationHeader* fragmentation,
> +                                  const webrtc::CodecSpecificInfo* codecSpecificInfo,
> +                                  int64_t renderTimeMs)
> +{
> +  CODEC_LOGD("this:%p will decode", this);

I suggest remove this log since Decode be trigger around 30ms.
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +497,5 @@
> +  // GraphicBuffer's size and actual video size is different.
> +  // See Bug 850566.
> +  layers::SurfaceDescriptorGralloc newDescriptor = descriptor->get_SurfaceDescriptorGralloc();
> +  newDescriptor.size() = gfx::IntSize(frame->mWidth, frame->mHeight);
> +

newDescriptor.size"()"?

@@ +563,5 @@
> +  if (timeUs < outTime) {
> +    // Buffer out of date.
> +    omx->releaseOutputBuffer(index);
> +  } else {
> +    webrtc::I420VideoFrame* video_frame = GenerateVideoFrame(

videoFrame
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +133,5 @@
> +      MonitorAutoLock lock(mMonitor);
> +      if (mOmx.get()) {
> +        mOmx->releaseOutputBuffer(mBufferIndex);
> +        mOmx = nullptr;
> +      }

suggestion: Call Unlock here and remove manually release code here

@@ +611,5 @@
> +    mOMX->mConfig->setInt32("slice-height", height);
> +    mOMX->mWidth = width;
> +    mOMX->mHeight = height;
> +    sp<Surface> nativeWindow = nullptr;
> +    if (mOMX->mNativeWindowClient.get()) {

What if mOMX->mNativeWindowClient.get() == nullptr? Should we keep configure mOMX whil nativewindowclient is null?

@@ +614,5 @@
> +    sp<Surface> nativeWindow = nullptr;
> +    if (mOMX->mNativeWindowClient.get()) {
> +      nativeWindow = new Surface(mOMX->mNativeWindowClient->getIGraphicBufferProducer());
> +    }
> +    mOMX->Configure(mOMX->mConfig, nativeWindow, nullptr, 0);

Make mNativeWindowClient be private and get nativewindow from this object inside Configure.
The first parameter of Configure looks weird to me, why we need to pass a data member of mOMX to mOMX's member function?
Comment on attachment 8399306 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +477,5 @@
> +  //       changes to stagefright code.
> +  sp<RefBase> obj;
> +  bool hasGraphicBuffer = decoded->meta()->findObject("graphic-buffer", &obj);
> +  MOZ_ASSERT(hasGraphicBuffer);
> +  if (!hasGraphicBuffer) {

nitty: move #480 into if(!hasXXX) statement

@@ +486,5 @@
> +  sp<GraphicBuffer> gb = static_cast<GraphicBuffer*>(obj.get());
> +  MOZ_ASSERT(gb.get());
> +  CODEC_LOGD("decode omx buffer:%p has graphic buffer:%p",
> +    decoded->data(), hasGraphicBuffer ? gb.get() : nullptr);
> +  if (!gb.get()) {

nitty: move #487 into if(!bg.get()) statement

@@ +538,5 @@
> +  size_t index;
> +  size_t outOffset;
> +  size_t outSize;
> +  int64_t outTime;
> +  uint32_t outFlags;

nitty: give initial value to these variables to prevent compiler's warning
(In reply to Randell Jesup [:jesup] from comment #24)
> Comment on attachment 8399306 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G.
> 
> Review of attachment 8399306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +171,5 @@
> > +    int cnt = ++mRefCnt;
> > +    return cnt;
> > +  }
> > +
> > +  int Release()
> 
> Why are we hand-rolling AddRef and Release here?

 Cannot use NS_INLINE_DECL_REFCOUNTING() here because return type conflict with webrtc::NativeHandle.
 Will use AutoRefCounted<> in patch v2.

> 
> @@ +207,5 @@
> > +  AMessage* format = new AMessage;
> > +
> > +  format->setString("mime", MEDIA_MIMETYPE_VIDEO_AVC);
> > +  format->setInt32("store-metadata-in-buffers", 0);
> > +  format->setInt32("prepend-sps-pps-to-idr-frames", 0);
> 
> Are we using sprop-parameter-sets in the SDP?  If not, we need to insert
> SPS/PPS NALUs

 I don't think we use sprop-parameter-sets, at least not currently.
 AFAIK, this setting is to insert SPS/PPS NALUs before each IDR frame. Since stagefright encoder always generates SPS/PPS NALUs as its 1st output, I don't think it need to be set.

> 
> @@ +208,5 @@
> > +
> > +  format->setString("mime", MEDIA_MIMETYPE_VIDEO_AVC);
> > +  format->setInt32("store-metadata-in-buffers", 0);
> > +  format->setInt32("prepend-sps-pps-to-idr-frames", 0);
> > +  format->setInt32("bitrate", codecSettings->minBitrate * 1000); // kbps->bps
> 

 Actually this setting here doesn't effect encoder at all because we now use OMXVideoEncoder and it always set bitrate to 2kbps initially.

> 
> @@ +213,5 @@
> > +  format->setInt32("width", codecSettings->width);
> > +  format->setInt32("height", codecSettings->height);
> > +  format->setInt32("stride", codecSettings->width);
> > +  format->setInt32("slice-height", codecSettings->height);
> > +  format->setFloat("frame-rate", (float)codecSettings->maxFramerate);
> 
> What assumptions does the OMX encoder draw from this setting?

 In my test, setting this or not doesn't seem to change the behavior of OMX encoder.

> 
> @@ +219,5 @@
> > +  // <B2G>/hardware/qcom/media/mm-video/vidc/venc/src/video_encoder_device.cpp:
> > +  // venc_dev::venc_set_color_format()
> > +  format->setInt32("color-format", OMX_COLOR_FormatYUV420SemiPlanar);
> > +  // FIXME: get correct parameters from codecSettings?
> > +  format->setInt32("i-frame-interval", 1); // one I-frame per sec
> 
> We should never send an iframe except as a result of packet loss or at the
> start of a stream (unless the encoder decides it's cheaper, like in a scene
> change perhaps)
> 

 The stagefright encoder will produce I-frame every 2 seconds even when this setting is missing.

> @@ +222,5 @@
> > +  // FIXME: get correct parameters from codecSettings?
> > +  format->setInt32("i-frame-interval", 1); // one I-frame per sec
> > +  format->setInt32("profile", OMX_VIDEO_AVCProfileBaseline);
> > +  format->setInt32("level", OMX_VIDEO_AVCLevel3);
> > +  //format->setInt32("bitrate-mode", OMX_Video_ControlRateConstant);
> 
> What would this do if enabled?  Please comment why it's not

 I haven't check the effect of this setting and decided to leave it to the default value in OMXVideoEncoder used by patch v2.

> 
> @@ +246,5 @@
> > +  MutexAutoLock lock(mMutex);
> > +
> > +  // TODO: eliminate extra pixel copy & color conversion
> > +  size_t size = codecSettings->width * codecSettings->height * 3 / 2;
> > +  if (mEncodedImage._size < size) {
> 
> An encoded image can be larger than the source image.  This should be a null
> pointer initially, and then a high water mark (or allocate a "reasonable"
> buffer to start, and realloc larger if needed).  See the mods made in
> response to reviews in bug 969395.

 Changes in patch v2 will remove this allocation and use buffer allocated for each frame in OMXVideoEncoder instead.

> 
> In fact, this code is *remarkably* similar to that code (before I had Qi
> modify it).  Is this cut-and-pasted from examples?  Or was that patch based
> on an earlier version of this one, or vice-versa?
> 

 You're *right*. Qi's patch and this one both derived from my experimental work that I shared to him, so lots of problems in these patches are my bad. :)

> 
> @@ +592,5 @@
> > +  if (inputImage._length== 0 || !inputImage._buffer) {
> > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > +  }
> > +
> > +  uint32_t time = PR_IntervalNow();
> 
> All the debugs for timing should be #ifdef DEBUG or perhaps #ifdef
> TIME_H264_OMX

 All timing log will be removed in patch v2.

> 
>
 Other comments will also be addressed in patch v2.
(In reply to C.J. Ku[:CJKu] from comment #38)
> Comment on attachment 8399306 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G.
> 
> Review of attachment 8399306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> 
> @@ +104,5 @@
> > +  sp<ALooper> mLooper;
> > +  sp<MediaCodec> mCodec; // OMXCodec
> > +  sp<AMessage> mConfig;
> > +  int mWidth;
> > +  int mHeight;
> 
> Do we really need these two variables?

 Unfortunately yes, since the correct image size is not carried in the arguments of Decode(), so we need to remember it ourselves.

> 
> @@ +333,5 @@
> > +  CODEC_LOGD("this:%p encode dequeue OMX output buffer len:%u time:%lld flags:0x%08x took %u ms",
> > +             this, output.Length(), timeUs, flags, PR_IntervalToMilliseconds(PR_IntervalNow()-time));
> > +
> > +  MutexAutoLock lock(mMutex);
> > +
> 
> Do we need check mEncoderImage._size here? Is that possible that
> WebrtcOMXH264VideoEncoder::Encode be called after
> WebrtcOMXH264VideoEncoder::Release()?

 mEncoderImage will be removed in patch v2. And AFAIK Encode() will never be called after Release(). 

> 
> @@ +420,5 @@
> > +    mOMX->mConfig = VideoCodecSettings2AMessage(codecSettings);
> > +    mOMX->mNativeWindow = new GonkNativeWindow();
> > +    mOMX->mNativeWindow->setNewFrameCallback(mOMX);
> > +    mOMX->mNativeWindowClient =
> > +        new GonkNativeWindowClient(mOMX->mNativeWindow->getBufferQueue());
> 
> Move 420 ~ 424 into WebrtcOMXEncoder::Init function.
> Even if we are going to refactory this part in Bug 984223, we should still
> do initial task in correct place.
> 
> Code here should be very simple
>  mOMX = new WebrtcOMXDecoder(MEDIA_MIMETYPE_VIDEO_AVC);
>  mOMX->Initiate(codecSettings);

 In patch v2, most part of configuration code here will be moved into member function of mOMX.

> @@ +610,5 @@
> > +    mOMX->mConfig->setInt32("stride", width);
> > +    mOMX->mConfig->setInt32("slice-height", height);
> > +    mOMX->mWidth = width;
> > +    mOMX->mHeight = height;
> > +    sp<Surface> nativeWindow = nullptr;

 Same as above.

> 
> Can we move #598~#614 into mOMX->Configure?
> If I am a maintainer of WebrtcOMXH264VideoDecoder, I really don't want to
> know how to setup mConfig on mOMX

 Same as above.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.h
> @@ +26,5 @@
> > +#include "mozilla/Mutex.h"
> > +
> > +#include "MediaConduitInterface.h"
> > +#include "AudioConduit.h"
> > +#include "VideoConduit.h"
> 
> Do we really include these three headers here?

 VideoConduit.h is needed for WebrtcVideo(Encoder|Decoder) and unfortunately it won't compile without AudioConduit.h.
 MediaConduitInterface.h will be removed in patch v2.

> 
>
 Other comments will also be addressed in patch v2.
Many changes in this version:
- update gralloc buffer mangement code for bug 985772
- use seperate thread to dequeue output buffers so it won't block decode thread. This also makes output data available sooner than before.
- fix ICS build break
- address review comments & further code clean up
 * Mozilla coding style
 * turn static functions used by decoder into member functions
 * hide previously exposed data members
 * get rid of unused code/variables in configuration (originally for encoder)
Attachment #8399306 - Attachment is obsolete: true
Attachment #8399306 - Flags: review?(ekr)
Attachment #8403944 - Flags: review?(rjesup)
Attachment #8403944 - Flags: feedback?(cku)
Attachment #8403944 - Flags: feedback?(ayang)
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +72,5 @@
> +  virtual int Release() MOZ_OVERRIDE
> +  {
> +    AtomicRefCounted<ImageNativeHandle>::Release();
> +  // Return value is not 100% accurate if preempted here but scoped_refptr
> +  // doesn't really care about that anyway.

indent

@@ +128,5 @@
> +  void Stop() {
> +    if (mThread != nullptr) {
> +      mThread->Shutdown();
> +      mThread = nullptr;
> +    }

Assert Start and Stop be executed in the same thread. Otherwise, mutex is needed

@@ +148,5 @@
> +    if (!mInputFrames.empty()) {
> +      CODEC_LOGD("Try to drain frame");
> +      EncodedFrame frame = mInputFrames.front();
> +      bool shouldPop = false;
> +      { // Unlock while draining because it's blocking.

// DrainOutput takes time and there is no reason to block mInputFrames 
// while DrainOutput pulling data from MediaCodec.

separate comment into a second line.
I like the design here :)

@@ +158,5 @@
> +      }
> +    }
> +
> +    mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +

Is there a special reason that dispatch run task again here? want to yield CPU to other thread or easy to Stop?
Why not just use a infinite while loop here?

@@ +166,5 @@
> +protected:
> +  OMXOutputDrain()
> +    : mThread(nullptr)
> +    , mMutex("OMXOutputDrain Lock")
> +    , mInputFrames()

mInputFrames() mThread(nullptr) are not required
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

ChiaJung, please help to review nativewindow and textureclient relative code in WebrtcOMXH264VideoCodec.cpp, thanks
Attachment #8403944 - Flags: feedback?(chung)
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +113,5 @@
> +  uint32_t mRenderTimeMs;
> +};
> +
> +// Base runnable class to repeatly pull OMX output buffers in seperate thread.
> +class OMXOutputDrain : public nsRunnable

OMXOutputDrainer?

@@ +138,5 @@
> +    MutexAutoLock lock(mMutex);
> +    mInputFrames.push(aFrame);
> +  }
> +
> +  NS_IMETHODIMP Run()

MOZ_OVERRIDE

@@ +258,5 @@
> +    uint32_t flags = 0;
> +    if (aEncoded._frameType == webrtc::kKeyFrame) {
> +      flags = aIsFirstFrame ? MediaCodec::BUFFER_FLAG_CODECCONFIG : MediaCodec::BUFFER_FLAG_SYNCFRAME;
> +    }
> +    size_t size = aEncoded._length;

MOZ_ASSERT(size);
to protect our assumption (size is always larger then 0)

@@ +310,5 @@
> +      MOZ_ASSERT(err == OK);
> +      err = INFO_OUTPUT_BUFFERS_CHANGED;
> +    } else if (err != OK) {
> +      CODEC_LOGE("decode dequeue OMX output buffer error:%d", err);
> +    }

there aretwo if statements and two else if statements here. Use switch might be better
case -EAGAIN/INFO_FORMAT_CHANGED/INFO_OUTPUT_BUFFERS_CHANGED/OK and default

Is there a way to check the size of mInputFrames and enqueue buffer numbers of media codec are the same? Any wrong logic, int the future, fails this assumption cause big problem

@@ +329,5 @@
> +
> +    if (aCallback) {
> +      aCallback->Decoded(*videoFrame);
> +    }
> +

if (videoFrame == nullptr) {
  mCodec->releaseOutputBuffer(index);
}
else {
  // ... or OMX buffer will be released by RecycleCallback after rendered.
  if (aCallback) {
    aCallback->Decoded(*videoFrame);
  }
}

@@ +371,5 @@
> +    return err;
> +  }
> +
> +  status_t Stop()
> +  {

assertion of Start/Stop executed on the same thread

@@ +386,5 @@
> +
> +    status_t err = mCodec->stop();
> +    if (err == OK) {
> +      mStarted = false;
> +    }

What if err != OK? mStarted still keep true?
Reset mInputBuffers & mOutputBuffers

@@ +659,5 @@
> +  CODEC_LOGD("WebrtcOMXH264VideoEncoder:%p set bitrate:%u, frame rate:%u)",
> +             this, aBitRate, aFrameRate);
> +  if (!mOMX) {
> +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> +  }

MOZ_ASSERT(mOMX) since you don't really want user call this API before InitEncode
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +131,5 @@
> +      mThread = nullptr;
> +    }
> +  }
> +
> +  void QueueInput(EncodedFrame& aFrame)

const EncodedFrame& aFrame

@@ +269,5 @@
> +
> +    CODEC_LOGD("decode queue input buffer len:%u flags:%u time:%lld",
> +      size, flags, *aInputTimeUs);
> +
> +    if (err == OK && !(flags & MediaCodec::BUFFER_FLAG_CODECCONFIG)) {

if (err == OK && (flags & MediaCodec::BUFFER_FLAG_SYNCFRAME)) {
We QueueInput only when flags & MediaCodec::BUFFER_FLAG_SYNCFRAME and discard the other two cases(BUFFER_FLAG_CODEC_CONFIG or BUFFER_FLAG_END_OF_STREAM)

@@ +610,5 @@
> +WebrtcOMXH264VideoEncoder::RegisterEncodeCompleteCallback(
> +    webrtc::EncodedImageCallback* aCallback)
> +{
> +  CODEC_LOGD("WebrtcOMXH264VideoEncoder:%p set callback:%p", this, aCallback);
> +  mCallback = aCallback;

MOZ_ASSERT(aCallback)

@@ +617,5 @@
> +}
> +
> +int32_t
> +WebrtcOMXH264VideoEncoder::Release()
> +{

WebrtcOMXH264VideoEncoder::Encode/WebrtcOMXH264VideoEncoder::Release thread safe

@@ +721,5 @@
> +
> +int32_t
> +WebrtcOMXH264VideoDecoder::RegisterDecodeCompleteCallback(
> +    webrtc::DecodedImageCallback* aCallback)
> +{

MOZ_ASSERT(aCallback);

@@ +731,5 @@
> +int32_t
> +WebrtcOMXH264VideoDecoder::Release()
> +{
> +  CODEC_LOGD("WebrtcOMXH264VideoDecoder:%p will be released", this);
> +

WebrtcOMXH264VideoDecoder::Decode/WebrtcOMXH264VideoDecoder::Release/WebrtcOMXH264VideoDecoder::Reset() need to be called on the same thread or re-entry thread safe quarantee  

In this call sequence, module crashs

WebrtcOMXH264VideoDecoder::Decode(#710)
WebrtcOMXH264VideoDecoder::Release
WebrtcOMXH264VideoDecoder::Decode(#713)
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

John, to optimize performance, you introduce two new drainer thread to reduce encode/ decode latency, which is really good.
The only concern from me is thread safe issue, please check my previews comment and thanks for your hard work.
Attachment #8403944 - Flags: feedback?(cku) → feedback+
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.h
@@ +46,5 @@
> +  virtual int32_t SetChannelParameters(uint32_t aPacketLossRate,
> +                                       int aRoundTripTimeMs);
> +
> +  virtual int32_t SetRates(uint32_t aBitRate, uint32_t aFrameRate);
> +

verbose ... MOZ_OVERRIDE for all these virtual functions
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +144,5 @@
> +    CODEC_LOGD("Run output drain");
> +
> +    MutexAutoLock lock(mMutex);
> +
> +    if (!mInputFrames.empty()) {

Add a new function to wait has-item event here 
mInputFrames.waitHasItem();
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

NativeWindow/TextureClient part seems great for me.

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +334,5 @@
> +    return err;
> +  }
> +
> +private:
> +  class OutputDrain : public OMXOutputDrain

Why polling here rather than listen to OnNewFrame callback from GonkNativeWindow?

::: widget/gonk/nativewindow/moz.build
@@ +45,1 @@
>      if CONFIG['ANDROID_VERSION'] == '19':

what happend if !MOZ_B2G_CAMERA and !MOZ_OMX_DECODER and MOZ_WEBRTC?
Attachment #8403944 - Flags: feedback?(chung) → feedback+
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +113,5 @@
> +  uint32_t mRenderTimeMs;
> +};
> +
> +// Base runnable class to repeatly pull OMX output buffers in seperate thread.
> +class OMXOutputDrain : public nsRunnable

OMXOutputDrain should move to header file for the following integration with bug 984223
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

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

nice!

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +393,5 @@
> +  }
> +
> +  webrtc::I420VideoFrame*
> +  GenerateVideoFrame(EncodedFrame& aEncoded, uint32_t aBufferIndex,
> +                       const sp<ABuffer>& aOMXBuffer)

indent

@@ +492,5 @@
> +    encoded._buffer = output.Elements();
> +    encoded._completeFrame = true;
> +    bool isParamSets = (flags & MediaCodec::BUFFER_FLAG_CODECCONFIG);
> +    encoded._frameType = (isParamSets || (flags & MediaCodec::BUFFER_FLAG_SYNCFRAME)) ?
> +                         webrtc::kKeyFrame : webrtc::kDeltaFrame;

indent.

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

indent.

@@ +589,5 @@
> +    if (!mOutputDrain) {
> +      mOutputDrain = new EncOutputDrain(mOMX, mCallback,
> +                                          aInputImage.width(),
> +                                          aInputImage.height(),
> +                                          &stats_, &stampers_);

indent.

@@ +697,5 @@
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  // 1st input carries SPS/PPS NALUs.
> +  bool firstFrame = !mOMX;

initialized?

@@ +720,5 @@
> +}
> +
> +int32_t
> +WebrtcOMXH264VideoDecoder::RegisterDecodeCompleteCallback(
> +    webrtc::DecodedImageCallback* aCallback)

indent
Comment on attachment 8403944 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G. Take 2.

>+  NS_IMETHODIMP Run()
>+  {
>+    CODEC_LOGD("Run output drain");
>+
>+    MutexAutoLock lock(mMutex);
>+
>+    if (!mInputFrames.empty()) {
>+      CODEC_LOGD("Try to drain frame");
>+      EncodedFrame frame = mInputFrames.front();
>+      bool shouldPop = false;
>+      { // Unlock while draining because it's blocking.
>+        MutexAutoUnlock unlock(mMutex);
>+        shouldPop = DrainOutput(frame);
>+      }
>+      if (shouldPop) {
>+        mInputFrames.pop();
>+      }
>+    }
>+
>+    mThread->Dispatch(this, NS_DISPATCH_NORMAL);

That looks like busy thread.

>+
>+    return NS_OK;
>+  }
>+
>+protected:
>+  OMXOutputDrain()
>+    : mThread(nullptr)
>+    , mMutex("OMXOutputDrain Lock")
>+    , mInputFrames()
>+  {}
>+
>+  // Drain output buffer for input frame aFrame. Return true to indicate we
>+  // should pop input queue, and return false to indicate aFrame should not be
>+  // removed from input queue (either it fails to drain or the drained output
>+  // is SPS/PPS NALUs that has no corresponding input in queue).
>+  virtual bool DrainOutput(EncodedFrame& aFrame) = 0;
>+
>+  nsCOMPtr<nsIThread> mThread;
>+  std::queue<EncodedFrame> mInputFrames;
>+  Mutex mMutex; // To protect mInputFrames.
>+};
>+
>+// H.264 decoder using stagefright.
>+class WebrtcOMXDecoder MOZ_FINAL
>+{
>+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WebrtcOMXDecoder)
>+public:
>+  WebrtcOMXDecoder(const char* aMimeType)
>+    : mWidth(0)
>+    , mHeight(0)
>+    , mStarted(false)
>+    , mOutputDrain(nullptr)
>+  {
>+    // Create binder thread pool required by stagefright.
>+    android::ProcessState::self()->startThreadPool();
>+
>+    mLooper = new ALooper;
>+    mLooper->start();
>+    mCodec = MediaCodec::CreateByType(mLooper, aMimeType, false /* encoder */);
>+  }
>+
>+  virtual ~WebrtcOMXDecoder()
>+  {
>+    Stop();
>+    mCodec->release();
>+    mCodec.clear();
>+    mLooper.clear();
>+  }

Shouldn't we add MOZ_COUNT_CTOR/MOZ_COUNT_DTOR on constructor/destructor?

>+private:
>+  class OutputDrain : public OMXOutputDrain
>+  {
>+  public:
>+    OutputDrain(WebrtcOMXDecoder* aOMX, webrtc::DecodedImageCallback* aCallback)
>+      : mOMX(aOMX)
>+      , mCallback(aCallback)
>+    {}
>+
>+  protected:
>+    virtual bool DrainOutput(EncodedFrame& aFrame) MOZ_OVERRIDE
>+    {
>+      return (mOMX->DrainOutput(aFrame, mCallback) == OK);
>+    }
>+
>+  private:
>+    RefPtr<WebrtcOMXDecoder> mOMX;
>+    webrtc::DecodedImageCallback* mCallback;
>+  };
>+
>+  status_t Start()
>+  {
>+    MOZ_ASSERT(!mStarted);
>+    if (mStarted) {
>+      return OK;
>+    }
>+
>+    status_t err = mCodec->start();
>+    if (err == OK) {
>+      mStarted = true;
>+      mCodec->getInputBuffers(&mInputBuffers);
>+      mCodec->getOutputBuffers(&mOutputBuffers);
>+    }
>+
>+    return err;
>+  }
>+
>+  status_t Stop()
>+  {
>+    MOZ_ASSERT(mStarted);
>+    if (!mStarted) {
>+      return OK;
>+    }
>+
>+    if (mOutputDrain) {
>+      mOutputDrain->Stop();
>+      delete mOutputDrain;

I'd prefer to use nsAutoPtr here, not delete. Same as other places.

>+int32_t
>+WebrtcOMXH264VideoEncoder::Encode(
>+    const webrtc::I420VideoFrame& aInputImage,
>+    const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
>+    const std::vector<webrtc::VideoFrameType>* aFrameTypes)
>+{
>+  MOZ_ASSERT(mOMX);
>+  if (!mOMX) {
>+    return WEBRTC_VIDEO_CODEC_ERROR;
>+  }
>+  CODEC_LOGD("WebrtcOMXH264VideoEncoder:%p will encode", this);
>+
>+  if (!mOMXConfigured) {
>+    mOMX->Configure(mWidth, mHeight, mFrameRate,
>+                    OMXVideoEncoder::BlobFormat::AVC_NAL);
>+    mOMXConfigured = true;
>+    CODEC_LOGD("WebrtcOMXH264VideoEncoder:%p start OMX with image size:%ux%u",
>+               this, mWidth, mHeight);
>+  }
>+
>+  // Wrap I420VideoFrame input with PlanarYCbCrImage for OMXVideoEncoder.
>+  layers::PlanarYCbCrData yuvData;
>+  yuvData.mYChannel = const_cast<uint8_t*>(aInputImage.buffer(webrtc::kYPlane));
>+  yuvData.mYSize = gfx::IntSize(aInputImage.width(), aInputImage.height());
>+  yuvData.mYStride = aInputImage.stride(webrtc::kYPlane);
>+  MOZ_ASSERT(aInputImage.stride(webrtc::kUPlane) == aInputImage.stride(webrtc::kVPlane));
>+  yuvData.mCbCrStride = aInputImage.stride(webrtc::kUPlane);
>+  yuvData.mCbChannel = const_cast<uint8_t*>(aInputImage.buffer(webrtc::kUPlane));
>+  yuvData.mCrChannel = const_cast<uint8_t*>(aInputImage.buffer(webrtc::kVPlane));
>+  yuvData.mCbCrSize = gfx::IntSize((yuvData.mYSize.width + 1) / 2,
>+                                   (yuvData.mYSize.height + 1) / 2);
>+  yuvData.mPicSize = yuvData.mYSize;
>+  yuvData.mStereoMode = StereoMode::MONO;
>+  layers::PlanarYCbCrImage img(nullptr);
>+  img.SetDataNoCopy(yuvData);
>+
>+  nsresult rv = mOMX->Encode(&img,
>+                             yuvData.mYSize.width,
>+                             yuvData.mYSize.height,
>+                             aInputImage.timestamp() * 1000 / 90, // 90kHz -> us.

From my understanding, codec doesn't care about timestamp but render cares. Codec needs to pass this value exactly to output frame. So I don't think we need to convert time unit here.

>+
>+class WebrtcOMXH264VideoEncoder : public WebrtcVideoEncoder
>+{
>+public:
>+  WebrtcOMXH264VideoEncoder();
>+
>+  virtual ~WebrtcOMXH264VideoEncoder();
>+
>+  // Implement VideoEncoder interface.
>+  virtual int32_t InitEncode(const webrtc::VideoCodec* aCodecSettings,
>+                             int32_t aNumOfCores,
>+                             uint32_t aMaxPayloadSize);
>+
>+  virtual int32_t Encode(const webrtc::I420VideoFrame& aInputImage,
>+                         const webrtc::CodecSpecificInfo* aCodecSpecificInfo,
>+                         const std::vector<webrtc::VideoFrameType>* aFrameTypes);
>+
>+  virtual int32_t RegisterEncodeCompleteCallback(webrtc::EncodedImageCallback* aCallback);
>+
>+  virtual int32_t Release();
>+
>+  virtual int32_t SetChannelParameters(uint32_t aPacketLossRate,
>+                                       int aRoundTripTimeMs);
>+
>+  virtual int32_t SetRates(uint32_t aBitRate, uint32_t aFrameRate);
>+
>+private:
>+  android::OMXVideoEncoder* mOMX;
>+  webrtc::EncodedImageCallback* mCallback;
>+  OMXOutputDrain* mOutputDrain;
>+  uint32_t mWidth;
>+  uint32_t mHeight;
>+  uint32_t mFrameRate;
>+  bool mOMXConfigured;
>+  webrtc::EncodedImage mEncodedImage;
>+};
>+
>+class WebrtcOMXH264VideoDecoder : public WebrtcVideoDecoder
>+{
>+public:
>+  WebrtcOMXH264VideoDecoder();
>+
>+  virtual ~WebrtcOMXH264VideoDecoder();
>+
>+  // Implement VideoDecoder interface.
>+  virtual int32_t InitDecode(const webrtc::VideoCodec* aCodecSettings,
>+                             int32_t aNumOfCores);
>+  virtual int32_t Decode(const webrtc::EncodedImage& aInputImage,
>+                         bool aMissingFrames,
>+                         const webrtc::RTPFragmentationHeader* aFragmentation,
>+                         const webrtc::CodecSpecificInfo* aCodecSpecificInfo = nullptr,
>+                         int64_t aRenderTimeMs = -1);
>+  virtual int32_t RegisterDecodeCompleteCallback(webrtc::DecodedImageCallback* callback);
>+
>+  virtual int32_t Release();

Release() has been declared in 2 base classes of WebrtcVideoDecoder.
It will be error when use an auto ref ptr to hold this instance.
Encoder has the same problem.
Attachment #8403944 - Flags: feedback?(ayang)
- fix busy looping bug in OMXOutputDrain. Instead of running repeatedly, it now only schedule one run per input 
- remove ref counting macro in mozilla::Video(Encoder|Decoder) because Release() will conflict with webrtc:Video(Encoder|Decoder)::Release()
- addresses some other feekbacks in v2
Attachment #8403944 - Attachment is obsolete: true
Attachment #8403944 - Flags: review?(rjesup)
Attachment #8404808 - Flags: review?(rjesup)
Comment on attachment 8404808 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v2.1.

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

nitty

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +176,5 @@
> +
> +    return NS_OK;
> +  }
> +
> +  ~OMXOutputDrain() {

MOZ_OVERRIDE

@@ +218,5 @@
> +    mLooper->start();
> +    mCodec = MediaCodec::CreateByType(mLooper, aMimeType, false /* encoder */);
> +  }
> +
> +  virtual ~WebrtcOMXDecoder()

No need to be virtual since it's final
Lets try to land this asap. We have partners who want to do optimization work based on this.
Comment on attachment 8399298 [details] [diff] [review]
Part 2: Support 'handle-using' video frames for WebRTC on B2G.

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

I'm concerned th

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ +84,5 @@
>    virtual void RenderVideoFrame(const unsigned char* buffer,
>                                  unsigned int buffer_size,
>                                  uint32_t time_stamp,
> +                                int64_t render_time,
> +                                void* handle) = 0;

I share Jesup's concern that this is just a void *. How is the consumer of this supposed to know what it is?

Let's introduce a new type here and translate in the conduit.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +41,5 @@
>  #include "libyuv/convert.h"
>  #include "mozilla/gfx/Point.h"
>  #include "mozilla/gfx/Types.h"
>  
> +#include "webrtc/common_video/interface/native_handle.h"

Please don't use webrtc internals here. The whole point of VideoConduit was to isolate us from WebRTC. I'm not sure why the other versions got introduced in
bug 786234  but please don't propagate it further.

@@ +1411,5 @@
> +    void* handle) {
> +  if (!buffer && !handle) {
> +    // Nothing to render.
> +    MOZ_MTLOG(ML_ERROR, "Empty video frame");
> +    return;

When does this happen?

@@ +1446,5 @@
> +  else {
> +    MOZ_ASSERT(handle);
> +    // There should be a layers::Image* in |handle|
> +    webrtc::NativeHandle* h = static_cast<webrtc::NativeHandle*>(handle);
> +    image_ = static_cast<layers::Image*>(h->GetHandle());

Why aren't you translating this in VideoConduit?
Attachment #8399298 - Flags: review?(ekr) → review-
Attachment #8399326 - Flags: review?(ekr) → review+
Comment on attachment 8399309 [details] [diff] [review]
Part 6: Make H.264 preferred video codec when enabled in preferences.

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

::: media/webrtc/signaling/src/sipcc/core/common/prot_configmgr.c
@@ +632,5 @@
>        count++;
>      }
> +    if ( codec_mask & VCM_CODEC_RESOURCE_VP8) {
> +      aSupportedCodecs[count] = RTP_VP8;
> +      count++;

Here's what I propose we do:

1. Leave this code as-is so that we prefer H.264 to VP8. The reasoning here is that the only ffox versions which currently support H.264 are either (a) B2G or (b) development versions with OpenH264. In either case you want H.264 because that's what you are working on and as you say the language isn't expressive enough to have a conditional here.

2. Don't land the preference just yet, so that you need to turn on H.264 explicitly. That way people don't accidentally get H.264 in the current state when it doesn't work well.

3. Once H.264 performance is acceptable, we can revisit the question of preferences at the same time as we turn it on.

My sense is that we will probably want Mobile-Mobile to prefer H.264 but desktop to prefer VP8. So, that means that:

- Mobile should offer H.264 first and then VP8 and prefer H.264 if it's offered at all.
- Desktop should offer VP8 first and then H.264 and defer to the offerer's preferences.

(3264 requires that PTs be listed in order of preference.)

As you say, this will require modifying the code somewhat.
Attachment #8399309 - Flags: review?(ekr) → review+
Comment on attachment 8404808 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v2.1.

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

Please do not name these "Ext*" because we are about to introduce Gmp which is also external.

the convention here is "OMXH264VideoCodec.*"
Comment on attachment 8399308 [details] [diff] [review]
Part 5: Register H.264 external codec for B2G.

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

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +77,5 @@
>                                       mMaxFrameSize(0),
>                                       mMaxFrameRate(0),
>                                       mLoadManager(load_manager)
>    {
> +    // Replace codec name here because  WebRTC.org code has a whitelist of

This is fine for now, but please file a bug for this so we can fix it.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2126,5 @@
> +  // whitelist of supported video codec in |webrtc::ViECodecImpl::CodecValid()|
> +  // and will reject registration of those not in it.
> +  if (config->mName != "I420") {
> +    return 0;
> +  }

Why is this return 0? This is an error, no?

In fact, you should MOZ_ASSERT here.

@@ +2131,5 @@
> +  // Register H.264 codec.
> +  if (send) {
> +    VideoEncoder* encoder = ExtVideoCodec::CreateEncoder(ExtVideoCodec::CodecType::CODEC_H264);
> +    if (encoder) {
> +      conduit->SetExternalSendCodec(config->mType, encoder);

Check for errors.

@@ +2132,5 @@
> +  if (send) {
> +    VideoEncoder* encoder = ExtVideoCodec::CreateEncoder(ExtVideoCodec::CodecType::CODEC_H264);
> +    if (encoder) {
> +      conduit->SetExternalSendCodec(config->mType, encoder);
> +    }

This needs to return errors if the creates fail
Attachment #8399308 - Flags: review?(ekr) → review-
Comment on attachment 8404808 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v2.1.

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

This is close.

Three issues are not simply nits or addressed without re-review:
a) figuring out the correct looping behavior for OMXOutputDrain, or document why it must be as it is
b) handling resolution changes.  If this can't be trivially addressed, file a followup, block the H.264 meta bug 984239 on that, and add a check that the input resolution matches the configured, and if not drop the frame.  With that, we don't need to block on landing this
c) assumption that SPS/PPS are the first inputs

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +146,5 @@
> +
> +    MutexAutoLock lock(mLock);
> +    mInputFrames.push(aFrame);
> +    // Schedule draining for this input.
> +    mThread->Dispatch(this, NS_DISPATCH_NORMAL);

Please note that this can mean the Runnable (this) can be in the Event Queue for the thread multiple times!

While workable (with care), we may want to consider a different solution to avoid allocations, etc.  (like an mRunning boolean; if combined with a loop on input frames in Run() you can avoid extra Dispatches)

@@ +167,5 @@
> +        mInputFrames.pop();
> +      } else {
> +        // Schedule another run since input queue is not empty.
> +        mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +      }

This sequence *seems* wrong (or at least trouble-prone).  It's not actually checking the input queue state, so the queue could build up leaving permanent delay.

I'd suggest something more like: 
  if (shouldPop) { mInputFrames.pop(); }
  if (!mInputFrames.empty()) { Dispatch(); }

Also, why Dispatch to self instead of just looping until mInputFrames is empty?  Normally self-Dispatch is used to avoid responding to other events for "too long"; with a dedicated task here that's not relevant.

If there's a usage pattern that requires this type of setup, please add comments here to that effect

@@ +195,5 @@
> +
> +private:
> +  nsCOMPtr<nsIThread> mThread;
> +  std::queue<EncodedFrame> mInputFrames;
> +  Mutex mLock; // To protect mInputFrames.

reorder to put a blank line before mLock, and move mInputFrames to after it, to emphasize what's protected by the lock.

@@ +207,5 @@
> +  WebrtcOMXDecoder(const char* aMimeType)
> +    : mWidth(0)
> +    , mHeight(0)
> +    , mStarted(false)
> +    , mOutputDrain(nullptr)

not needed

@@ +209,5 @@
> +    , mHeight(0)
> +    , mStarted(false)
> +    , mOutputDrain(nullptr)
> +  {
> +    MOZ_COUNT_CTOR(WebrtcOMXDecoder);

Not needed if it's NS_INLINE_DECL_THREADSAFE_REFCOUNTING() (or NS_IMPL_ADDREF).  Only for non-refcounted objects.

@@ +220,5 @@
> +  }
> +
> +  virtual ~WebrtcOMXDecoder()
> +  {
> +    MOZ_COUNT_DTOR(WebrtcOMXDecoder);

Not needed

@@ +221,5 @@
> +
> +  virtual ~WebrtcOMXDecoder()
> +  {
> +    MOZ_COUNT_DTOR(WebrtcOMXDecoder);
> +    Stop();

Stop() assumes it has been Start()ed.  Perhaps "if (mStarted) { Stop(); }"

@@ +283,5 @@
> +    MOZ_ASSERT(size);
> +    const sp<ABuffer>& omxIn = mInputBuffers.itemAt(index);
> +    MOZ_ASSERT(omxIn->capacity() >= size);
> +    omxIn->setRange(0, size);
> +    memcpy(omxIn->data(), aEncoded._buffer, size);

Wish we could avoid this copy.... If there's a way to do so in the API, at least comment about it and file a followup

@@ +335,5 @@
> +      default:
> +        CODEC_LOGE("decode dequeue OMX output buffer error:%d", err);
> +        break;
> +    }
> +    NS_ENSURE_TRUE(err == OK, err);

IIRC we're trying to move away from NS_ENSURE_....() macros, to NS_WARN_IF... and explicit return for any new code.  See nsDebug.h

@@ +564,5 @@
> +  if (!mOMX) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  if (!mOMXConfigured) {

Verify what happens if the input size changes (which it can).  Does the overlying code stop and restart the encoder?  (I think not.)  Or does it just pass it down to Encode()?  (I think so).

If it's passed down, if the mWidth or mHeight change, you need to re-configure the OMX encoder.  If that requires stopping and restarting it, you have to do that.

(Resolution can change depending on bandwidth and camera rotation, etc).

@@ +705,5 @@
> +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  // 1st input carries SPS/PPS NALUs.

No, first input *may* carry them:
a) could be negotiated SPS/PPS instead of in-stream
b) packet loss could lose them

Both cases must be handled

@@ +755,5 @@
> +int32_t
> +WebrtcOMXH264VideoDecoder::Reset()
> +{
> +  CODEC_LOGD("WebrtcOMXH264VideoDecoder:%p will be reset", this);
> +  return WEBRTC_VIDEO_CODEC_OK;

Log a warning that it's not actually being reset.
Attachment #8404808 - Flags: review?(rjesup) → review-
(In reply to Eric Rescorla (:ekr) from comment #61)
> Comment on attachment 8399298 [details] [diff] [review]
> Part 2: Support 'handle-using' video frames for WebRTC on B2G.
> 
> Review of attachment 8399298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm concerned th
> 
> ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
> @@ +84,5 @@
> >    virtual void RenderVideoFrame(const unsigned char* buffer,
> >                                  unsigned int buffer_size,
> >                                  uint32_t time_stamp,
> > +                                int64_t render_time,
> > +                                void* handle) = 0;
> 
> I share Jesup's concern that this is just a void *. How is the consumer of
> this supposed to know what it is?
> 
> Let's introduce a new type here and translate in the conduit.

 Okay. A typedef for void* was introduced since patch v2 and I'll move the translation code from MediaPipeline to VideoConduit in next version.

> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +41,5 @@
> >  #include "libyuv/convert.h"
> >  #include "mozilla/gfx/Point.h"
> >  #include "mozilla/gfx/Types.h"
> >  
> > +#include "webrtc/common_video/interface/native_handle.h"
> 
> Please don't use webrtc internals here. The whole point of VideoConduit was
> to isolate us from WebRTC. I'm not sure why the other versions got
> introduced in
> bug 786234  but please don't propagate it further.

 Okay.

> 
> @@ +1411,5 @@
> > +    void* handle) {
> > +  if (!buffer && !handle) {
> > +    // Nothing to render.
> > +    MOZ_MTLOG(ML_ERROR, "Empty video frame");
> > +    return;
> 
> When does this happen?

 Never, I guess. :) Will replace it with assertion.

> 
> @@ +1446,5 @@
> > +  else {
> > +    MOZ_ASSERT(handle);
> > +    // There should be a layers::Image* in |handle|
> > +    webrtc::NativeHandle* h = static_cast<webrtc::NativeHandle*>(handle);
> > +    image_ = static_cast<layers::Image*>(h->GetHandle());
> 
> Why aren't you translating this in VideoConduit?

 Will do.
(In reply to Eric Rescorla (:ekr) from comment #63)
> Comment on attachment 8404808 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v2.1.
> 
> Review of attachment 8404808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please do not name these "Ext*" because we are about to introduce Gmp which
> is also external.
> 
> the convention here is "OMXH264VideoCodec.*"

 The idea was to use ExtVideoCodec as the factory for all external codec we'll implement. :)
 Anyway, I'll rename it to OMXVideoCodec, in case bug 969395 wants to share it.
Blocks: 995884
(In reply to Eric Rescorla (:ekr) from comment #64)
> Comment on attachment 8399308 [details] [diff] [review]
> Part 5: Register H.264 external codec for B2G.
> 
> Review of attachment 8399308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
> @@ +77,5 @@
> >                                       mMaxFrameSize(0),
> >                                       mMaxFrameRate(0),
> >                                       mLoadManager(load_manager)
> >    {
> > +    // Replace codec name here because  WebRTC.org code has a whitelist of
> 
> This is fine for now, but please file a bug for this so we can fix it.

 Done. Bug 995884.

> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +2126,5 @@
> > +  // whitelist of supported video codec in |webrtc::ViECodecImpl::CodecValid()|
> > +  // and will reject registration of those not in it.
> > +  if (config->mName != "I420") {
> > +    return 0;
> > +  }
> 
> Why is this return 0? This is an error, no?
> 
> In fact, you should MOZ_ASSERT here.

 Since external codec registration is only needed for I420, for others we do nothing but return without error. Will add comment to make it more clear.

> 
> @@ +2131,5 @@
> > +  // Register H.264 codec.
> > +  if (send) {
> > +    VideoEncoder* encoder = ExtVideoCodec::CreateEncoder(ExtVideoCodec::CodecType::CODEC_H264);
> > +    if (encoder) {
> > +      conduit->SetExternalSendCodec(config->mType, encoder);
> 
> Check for errors.

 Will do in next version.

> 
> @@ +2132,5 @@
> > +  if (send) {
> > +    VideoEncoder* encoder = ExtVideoCodec::CreateEncoder(ExtVideoCodec::CodecType::CODEC_H264);
> > +    if (encoder) {
> > +      conduit->SetExternalSendCodec(config->mType, encoder);
> > +    }
> 
> This needs to return errors if the creates fail

 Will do.
(In reply to Randell Jesup [:jesup] from comment #65)
> Comment on attachment 8404808 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v2.1.
> 
> Review of attachment 8404808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +705,5 @@
> > +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > +  }
> > +
> > +  // 1st input carries SPS/PPS NALUs.
> 
> No, first input *may* carry them:
> a) could be negotiated SPS/PPS instead of in-stream
> b) packet loss could lose them
> 
> Both cases must be handled

 For a), is there a mechanism in webrtc.org code to pass negotiated data to decoder?
 And for b), what's the proper way to handle packet lost? Can we tell remote peer to re-send parameter sets?
Flags: needinfo?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #65)
> Comment on attachment 8404808 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v2.1.
> 
> Review of attachment 8404808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is close.
> 
> Three issues are not simply nits or addressed without re-review:
> a) figuring out the correct looping behavior for OMXOutputDrain, or document
> why it must be as it is
> b) handling resolution changes.  If this can't be trivially addressed, file
> a followup, block the H.264 meta bug 984239 on that, and add a check that
> the input resolution matches the configured, and if not drop the frame. 
> With that, we don't need to block on landing this
> c) assumption that SPS/PPS are the first inputs
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +146,5 @@
> > +
> > +    MutexAutoLock lock(mLock);
> > +    mInputFrames.push(aFrame);
> > +    // Schedule draining for this input.
> > +    mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> 
> Please note that this can mean the Runnable (this) can be in the Event Queue
> for the thread multiple times!

 True. The idea here is for each input, there will be exactly one run scheduled.
 I'm not familiar with Mozilla thread/runnable usage convention, is having same event scheduled multiple times in queue considered to be a bad thing?

> 
> While workable (with care), we may want to consider a different solution to
> avoid allocations, etc.  (like an mRunning boolean; if combined with a loop
> on input frames in Run() you can avoid extra Dispatches)
 
 I've considered using ReentrantMonitor to implement loop waiting on queue but didn't see significant benefits compared to Dispatch(). Avoiding allocations and Dispatch calls are good points, but I still think Dispatch() looks simpler. 

> 
> @@ +167,5 @@
> > +        mInputFrames.pop();
> > +      } else {
> > +        // Schedule another run since input queue is not empty.
> > +        mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> > +      }
> 
> This sequence *seems* wrong (or at least trouble-prone).  It's not actually
> checking the input queue state, so the queue could build up leaving
> permanent delay.
> 
> I'd suggest something more like: 
>   if (shouldPop) { mInputFrames.pop(); }
>   if (!mInputFrames.empty()) { Dispatch(); }
> 
> Also, why Dispatch to self instead of just looping until mInputFrames is
> empty?  Normally self-Dispatch is used to avoid responding to other events
> for "too long"; with a dedicated task here that's not relevant.

 My comment above Dispatch() is in fact misleading. What I was trying to say there was "Schedule another run to drain output for aFrame since DrainOutput() instructed us to keep it in the queue. It could be that either the drained output is SPS/PPS NALUs which has no corresponding input, or its output is not available (the timed-out case) yet". Not a good idea, indeed. :)
 You're right. Looping until queue is empty is better than Dispatch() here. Will change it in next version.

> 
> @@ +283,5 @@
> > +    MOZ_ASSERT(size);
> > +    const sp<ABuffer>& omxIn = mInputBuffers.itemAt(index);
> > +    MOZ_ASSERT(omxIn->capacity() >= size);
> > +    omxIn->setRange(0, size);
> > +    memcpy(omxIn->data(), aEncoded._buffer, size);
> 
> Wish we could avoid this copy.... If there's a way to do so in the API, at
> least comment about it and file a followup

 Unfortunately stagefright doesn't support externally allocated input buffers at the moment so the copy here seems inevitable.

> 
> @@ +564,5 @@
> > +  if (!mOMX) {
> > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > +  }
> > +
> > +  if (!mOMXConfigured) {
> 
> Verify what happens if the input size changes (which it can).  Does the
> overlying code stop and restart the encoder?  (I think not.)  Or does it
> just pass it down to Encode()?  (I think so).
> 
> If it's passed down, if the mWidth or mHeight change, you need to
> re-configure the OMX encoder.  If that requires stopping and restarting it,
> you have to do that.
> 
> (Resolution can change depending on bandwidth and camera rotation, etc).

 Yes, currently OMXVideoEncoder ignores input image that have different size than configured. rlin will take care of supporting input image resizing in OMX encoder, which is also required to support rotated input (bug 984215).

>
>

All other comments, except for missing SPS/PPS NALUs cases, will be addressed in next version.
Move webrtc::NativeHandle dependency from MediaPipeline to VideoConduit.
Attachment #8399298 - Attachment is obsolete: true
Attachment #8406116 - Flags: review?(ekr)
update commit message & carry r+ from jesup.
Attachment #8406121 - Flags: review+
Attachment #8400373 - Attachment is obsolete: true
Addresses r- comments.
Attachment #8399308 - Attachment is obsolete: true
Attachment #8406122 - Flags: review?(rjesup)
Attachment #8406122 - Flags: review?(ekr)
Address some r- comments:
- rename ExtVideoCodec factory class to OMXVideoCodec
- drain outputs for all already queued inputs in one run to save Dispatch() calls
Attachment #8404808 - Attachment is obsolete: true
Attachment #8406157 - Flags: review?(rjesup)
Attachment #8406157 - Flags: review?(ekr)
(In reply to John Lin[:jolin][:jhlin] from comment #69)

> > @@ +705,5 @@
> > > +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> > > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > > +  }
> > > +
> > > +  // 1st input carries SPS/PPS NALUs.
> > 
> > No, first input *may* carry them:
> > a) could be negotiated SPS/PPS instead of in-stream
> > b) packet loss could lose them
> > 
> > Both cases must be handled
> 
>  For a), is there a mechanism in webrtc.org code to pass negotiated data to
> decoder?

There should be; there's a CodecConfig structure and if we need to it can be extended.

>  And for b), what's the proper way to handle packet lost? Can we tell remote
> peer to re-send parameter sets?

b) Is almost never an issue for the codec.  The RTP code notices packet losses and requests either retransmissions (NACK) or PLI/etc to ask for a refresh.  If the sender is using SPS/PPS in-band (not in SDP), then on all IDRs it will send SPS/PPS/iframe.  The codec just has to try to decode packets as presented (even if there are gaps).  This only really comes into play if it gets an iframe and has never seen a PPS/SPS (via packets or config), or an iframe that references a PPS/SPS index it has never seen.  In that case, it simply doesn't decode the iframe and returns an error.

The calling code should notice an error and ask for a refresh from the sender (which catches the "lost packets on start of stream" case).
Flags: needinfo?(rjesup)
(In reply to Randell Jesup [:jesup] from comment #75)
> (In reply to John Lin[:jolin][:jhlin] from comment #69)
> 
> > > @@ +705,5 @@
> > > > +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> > > > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > > > +  }
> > > > +
> > > > +  // 1st input carries SPS/PPS NALUs.
> > > 
> > > No, first input *may* carry them:
> > > a) could be negotiated SPS/PPS instead of in-stream
> > > b) packet loss could lose them
> > > 
> > > Both cases must be handled
> > 
> >  For a), is there a mechanism in webrtc.org code to pass negotiated data to
> > decoder?
> 
> There should be; there's a CodecConfig structure and if we need to it can be
> extended.

 Just found a virtual function that seems to be used for out-of-band parameter sets (http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/codecs/interface/video_codec_interface.h#247). Will check that and implement it if necessary. 

> 
> >  And for b), what's the proper way to handle packet lost? Can we tell remote
> > peer to re-send parameter sets?
> 
> b) Is almost never an issue for the codec.  The RTP code notices packet
> losses and requests either retransmissions (NACK) or PLI/etc to ask for a
> refresh.  If the sender is using SPS/PPS in-band (not in SDP), then on all
> IDRs it will send SPS/PPS/iframe.  The codec just has to try to decode
> packets as presented (even if there are gaps).  This only really comes into
> play if it gets an iframe and has never seen a PPS/SPS (via packets or
> config), or an iframe that references a PPS/SPS index it has never seen.  In
> that case, it simply doesn't decode the iframe and returns an error.

 AFAICT, OMX encoder only generates SPS/PPS once (the 1st output). Do you think that we need to support sending SPS/PPS/iframe periodically (perhaps every n seconds or once per x frames)? 

> 
> The calling code should notice an error and ask for a refresh from the
> sender (which catches the "lost packets on start of stream" case).

 Okay. Will change Decode() to return uninitialized error when missing SPS/PPS.
Flags: needinfo?(rjesup)
(In reply to John Lin[:jolin][:jhlin] from comment #76)
> (In reply to Randell Jesup [:jesup] from comment #75)
> > (In reply to John Lin[:jolin][:jhlin] from comment #69)
> > 
> > > > @@ +705,5 @@
> > > > > +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> > > > > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > > > > +  }
> > > > > +
> > > > > +  // 1st input carries SPS/PPS NALUs.
> > > > 
> > > > No, first input *may* carry them:
> > > > a) could be negotiated SPS/PPS instead of in-stream
> > > > b) packet loss could lose them
> > > > 
> > > > Both cases must be handled
> > > 
> > >  For a), is there a mechanism in webrtc.org code to pass negotiated data to
> > > decoder?
> > 
> > There should be; there's a CodecConfig structure and if we need to it can be
> > extended.
> 
>  Just found a virtual function that seems to be used for out-of-band
> parameter sets
> (http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/codecs/interface/video_codec_interface.h#247). Will
> check that and implement it if necessary. 
> 
> > 
> > >  And for b), what's the proper way to handle packet lost? Can we tell remote
> > > peer to re-send parameter sets?
> > 
> > b) Is almost never an issue for the codec.  The RTP code notices packet
> > losses and requests either retransmissions (NACK) or PLI/etc to ask for a
> > refresh.  If the sender is using SPS/PPS in-band (not in SDP), then on all
> > IDRs it will send SPS/PPS/iframe.  The codec just has to try to decode
> > packets as presented (even if there are gaps).  This only really comes into
> > play if it gets an iframe and has never seen a PPS/SPS (via packets or
> > config), or an iframe that references a PPS/SPS index it has never seen.  In
> > that case, it simply doesn't decode the iframe and returns an error.
> 
>  AFAICT, OMX encoder only generates SPS/PPS once (the 1st output). Do you
> think that we need to support sending SPS/PPS/iframe periodically (perhaps
> every n seconds or once per x frames)? 
If receiver is OpenH264 decoder, I think sender should encode the keyframe with this type.
> 
> > 
> > The calling code should notice an error and ask for a refresh from the
> > sender (which catches the "lost packets on start of stream" case).
> 
>  Okay. Will change Decode() to return uninitialized error when missing
> SPS/PPS.
(In reply to Randy Lin [:rlin] from comment #77)

> > > 
> > > >  And for b), what's the proper way to handle packet lost? Can we tell remote
> > > > peer to re-send parameter sets?
> > > 
> > > b) Is almost never an issue for the codec.  The RTP code notices packet
> > > losses and requests either retransmissions (NACK) or PLI/etc to ask for a
> > > refresh.  If the sender is using SPS/PPS in-band (not in SDP), then on all
> > > IDRs it will send SPS/PPS/iframe.  The codec just has to try to decode
> > > packets as presented (even if there are gaps).  This only really comes into
> > > play if it gets an iframe and has never seen a PPS/SPS (via packets or
> > > config), or an iframe that references a PPS/SPS index it has never seen.  In
> > > that case, it simply doesn't decode the iframe and returns an error.
> > 
> >  AFAICT, OMX encoder only generates SPS/PPS once (the 1st output). Do you
> > think that we need to support sending SPS/PPS/iframe periodically (perhaps
> > every n seconds or once per x frames)? 

It should either generate SPS/PPS on every IDR (and the code layers above will request an IDR as needed), or if external-negotiated parameters sets are in use then no SPS/PPS should be generated.  Generating on the first IDR only probably is valid in the spec, but a bad idea in that loss of either will make the stream undecodeable.

> If receiver is OpenH264 decoder, I think sender should encode the keyframe
> with this type.

It should be configurable.

> > > The calling code should notice an error and ask for a refresh from the
> > > sender (which catches the "lost packets on start of stream" case).
> > 
> >  Okay. Will change Decode() to return uninitialized error when missing
> > SPS/PPS.

Good, thanks
Flags: needinfo?(rjesup)
Comment on attachment 8406157 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v2.2.

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

r- for the SPS/PPS configuration issue.

The frame rate issue is quite real; we need to be able to change the frame rate periodically.  We can land without it, but this will be needed very soon.

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +52,5 @@
> +  : public webrtc::NativeHandle
> +  , public AtomicRefCounted<ImageNativeHandle>
> +{
> +public:
> +  MOZ_DECLARE_REFCOUNTED_TYPENAME(ImageNativeHandle)

I should note that khuey is on a rampage to remove all RefCounted<>/AtomicRefCounted<> objects (lack of thread-safety and logging code), so this might need to change by the time it lands.

@@ +92,5 @@
> +    : mOmx(aOmx)
> +    , mBufferIndex(aBufferIndex)
> +  {}
> +
> +  static void ReturnOMXBuffer(layers::TextureClient* aClient, void* aClosure)

Is there any way we can use a dummy typedef here for void * so that attempts to pass other things to it will fail?

@@ +141,5 @@
> +  }
> +
> +  void QueueInput(const EncodedFrame& aFrame)
> +  {
> +    MOZ_ASSERT(mThread);

I presume Start/Stop/QueueInput must all be called from the same thread, since there's no locking.  Comment on that in the .h file please.

@@ +154,5 @@
> +  {
> +    MOZ_ASSERT(mThread);
> +
> +    MutexAutoLock lock(mLock);
> +    while (!mInputFrames.empty()) {

definitely better, though a monitor would still be a better solution.  This will work, however.

@@ +160,5 @@
> +      bool shouldPop = false;
> +      {
> +        // Unlock while draining because it's blocking.
> +        MutexAutoUnlock unlock(mLock);
> +        shouldPop = DrainOutput(frame);

Add a comment that the input frame is used to provide metadata

What if DrainOutput hits a real error?  There appears to be no way to stop trying to encode.

@@ +334,5 @@
> +        CODEC_LOGI("decode dequeue OMX output buffer timed out. Try later.");
> +        break;
> +      case INFO_FORMAT_CHANGED:
> +        // Not an error: will get this value when OMX output buffer is enabled.
> +        CODEC_LOGD("decode dequeue OMX output buffer format change");

would this include resolution changes?

@@ +678,5 @@
> +
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +// Note: stagefright doesn't handle frame rate change.

File a followup bug to reconfigure the codec (restart it if needed) on a frame rate change, since we need to do them for bitrate/load adaptation.  Put the bug number here.

@@ +725,5 @@
> +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  // 1st input carries SPS/PPS NALUs.

fix comment per previous review

@@ +731,5 @@
> +  if (firstFrame) {
> +    mOMX = new WebrtcOMXDecoder(MEDIA_MIMETYPE_VIDEO_AVC);
> +    // Get image width/height from parameter sets.
> +    sp<ABuffer> paramSets = new ABuffer(aInputImage._buffer, aInputImage._length);
> +    mOMX->ConfigureWithParamSets(paramSets);

This will fail if the first incoming packet isn't an SPS/PPS (and perhaps even if the order is swapped).
Attachment #8406157 - Flags: review?(rjesup) → review-
Attachment #8406122 - Flags: review?(rjesup) → review+
I realized I didn't hit Submit on this comment:

(In reply to John Lin[:jolin][:jhlin] from comment #70)
> (In reply to Randell Jesup [:jesup] from comment #65)

> > ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> > @@ +146,5 @@
> > > +
> > > +    MutexAutoLock lock(mLock);
> > > +    mInputFrames.push(aFrame);
> > > +    // Schedule draining for this input.
> > > +    mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> > 
> > Please note that this can mean the Runnable (this) can be in the Event Queue
> > for the thread multiple times!
> 
>  True. The idea here is for each input, there will be exactly one run
> scheduled.
>  I'm not familiar with Mozilla thread/runnable usage convention, is having
> same event scheduled multiple times in queue considered to be a bad thing?

It's not normal, but when done carefully it's not an error.

> > While workable (with care), we may want to consider a different solution to
> > avoid allocations, etc.  (like an mRunning boolean; if combined with a loop
> > on input frames in Run() you can avoid extra Dispatches)
>  
>  I've considered using ReentrantMonitor to implement loop waiting on queue
> but didn't see significant benefits compared to Dispatch(). Avoiding
> allocations and Dispatch calls are good points, but I still think Dispatch()
> looks simpler. 

Dispatch() of a single object does leave you vulnerable to making errors (though I saw none in the current code), since the normal paradigm for Runnables is:
 runnable = new FooRunnable(args);
 thread->Dispatch(runnable);
and the runnable releases any args or refs in args when it's done, and in some cases re-dispatches itself to the original thread if the args aren't threadsafe for releasing on another thread.

I think, deep down in the event queue code, each Dispatch requires an allocation, even if you keep reusing the same runnable.  And Dispatch is definitely heavier-weight than a reentrant monitor.  But Dispatch might be less code.

I'm ok with Dispatch, even with multiple Dispatches of the same runnable if it's documented(!).  But it may not be optimal - and that's probably ok.

Perhaps the biggest downside is if there is a buildup of Dispatches to the thread, then we can't shutdown the thread until ALL of them have been processed (we have to process a Shutdown event).  And that might be a problem.  If there's only a Dispatch when adding a frame to an empty queue (throw a lock around the queue, if it's not empty just add, if it's empty add and submit a runnable to wake up the encoder thread), then you can't bloat the Dispatch event queue and block shutdown.  Of course, a monitor could be used here too.

I'd prefer a monitor (not sure it needs to be reentrant), or a lock-and-dispatch-if-idle setup.


> > @@ +283,5 @@
> > > +    MOZ_ASSERT(size);
> > > +    const sp<ABuffer>& omxIn = mInputBuffers.itemAt(index);
> > > +    MOZ_ASSERT(omxIn->capacity() >= size);
> > > +    omxIn->setRange(0, size);
> > > +    memcpy(omxIn->data(), aEncoded._buffer, size);
> > 
> > Wish we could avoid this copy.... If there's a way to do so in the API, at
> > least comment about it and file a followup
> 
>  Unfortunately stagefright doesn't support externally allocated input
> buffers at the moment so the copy here seems inevitable.

Fine, just add a comment.

> 
> > 
> > @@ +564,5 @@
> > > +  if (!mOMX) {
> > > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > > +  }
> > > +
> > > +  if (!mOMXConfigured) {
> > 
> > Verify what happens if the input size changes (which it can).  Does the
> > overlying code stop and restart the encoder?  (I think not.)  Or does it
> > just pass it down to Encode()?  (I think so).
> > 
> > If it's passed down, if the mWidth or mHeight change, you need to
> > re-configure the OMX encoder.  If that requires stopping and restarting it,
> > you have to do that.
> > 
> > (Resolution can change depending on bandwidth and camera rotation, etc).
> 
>  Yes, currently OMXVideoEncoder ignores input image that have different size
> than configured. rlin will take care of supporting input image resizing in
> OMX encoder, which is also required to support rotated input (bug 984215).

That's the wrong approach.  The encoder must encode teh size it's given.  If OMX can't handle it, then you need to watch for a resolution change and reconfigure the encoder.  If need be (hopefully not), shutdown and restart the encoder.
Comment on attachment 8406157 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v2.2.

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

So, another issue that needs to be resolved.  What packetization mode is being used to send this data over RTP?  Mode 1?  

I think the encode/dequeue-encoded loop needs to be re-architected.  If it's single-large-NALU output (except for SPS/PPS NALs), then you can get away with this, probably, but it's not a good solution.  (mode 1 isn't necessarily bad or wrong to use in this context, but it has implications for the decoder side.  And even if it is used, assuming no dropped frames ever in the encoder is a dangerous assumption)

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +265,5 @@
> +
> +  status_t
> +  FillInput(const webrtc::EncodedImage& aEncoded, bool aIsFirstFrame,
> +            int64_t& aRenderTimeMs, webrtc::DecodedImageCallback* aCallback,
> +            int64_t* aInputTimeUs)

Why bother having aInputTimeUs since the returned value is thrown away?

@@ +528,5 @@
> +    }
> +
> +    // Tell base class not to pop input for SPS/PPS NALUs because they don't
> +    // have corresponding input.
> +    return !isParamSets;

Is the OMX encoder set to "generate a single large NALU" mode, with the packetization code set to fragment (H.264 SDP packetization-mode 1, Non-Interleaved mode)?  Otherwise, a single input frame may (usually will) generate multiple output NALUs which are sized to fit into a single packet (mode 0, Single NAL mode).

Perhaps it would be better to use the timestamp values coming out of the encoder, and either match/drop input packets by comparing to the output timestamps, or do a reset on resolution changes and cache width/height.

Also, it seems possible or likely that the encoder might decide to drop an input frame if it was temporarily far over bitrate (such as following an IDR), so relying on 1:1 input/output is not a good idea.  Perhaps the encoder doesn't in this setting - but it's bad to assume it can't.
(In reply to Randell Jesup [:jesup] from comment #79)
> Comment on attachment 8406157 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v2.2.
> 
> Review of attachment 8406157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the SPS/PPS configuration issue.
> 
> The frame rate issue is quite real; we need to be able to change the frame
> rate periodically.  We can land without it, but this will be needed very
> soon.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +52,5 @@
> > +  : public webrtc::NativeHandle
> > +  , public AtomicRefCounted<ImageNativeHandle>
> > +{
> > +public:
> > +  MOZ_DECLARE_REFCOUNTED_TYPENAME(ImageNativeHandle)
> 
> I should note that khuey is on a rampage to remove all
> RefCounted<>/AtomicRefCounted<> objects (lack of thread-safety and logging
> code), so this might need to change by the time it lands.

 Will fix in next version.

> 
> @@ +92,5 @@
> > +    : mOmx(aOmx)
> > +    , mBufferIndex(aBufferIndex)
> > +  {}
> > +
> > +  static void ReturnOMXBuffer(layers::TextureClient* aClient, void* aClosure)
> 
> Is there any way we can use a dummy typedef here for void * so that attempts
> to pass other things to it will fail?

 Will do.

> 
> @@ +141,5 @@
> > +  }
> > +
> > +  void QueueInput(const EncodedFrame& aFrame)
> > +  {
> > +    MOZ_ASSERT(mThread);
> 
> I presume Start/Stop/QueueInput must all be called from the same thread,
> since there's no locking.  Comment on that in the .h file please.

 Actually they're not. Will fix in next version.

> 
> @@ +154,5 @@
> > +  {
> > +    MOZ_ASSERT(mThread);
> > +
> > +    MutexAutoLock lock(mLock);
> > +    while (!mInputFrames.empty()) {
> 
> definitely better, though a monitor would still be a better solution.  This
> will work, however.

 Will rewrite to use monitor.

> 
> @@ +160,5 @@
> > +      bool shouldPop = false;
> > +      {
> > +        // Unlock while draining because it's blocking.
> > +        MutexAutoUnlock unlock(mLock);
> > +        shouldPop = DrainOutput(frame);
> 
> Add a comment that the input frame is used to provide metadata
> 
> What if DrainOutput hits a real error?  There appears to be no way to stop
> trying to encode.

 You're right. Will fix it in next version.

> 
> @@ +334,5 @@
> > +        CODEC_LOGI("decode dequeue OMX output buffer timed out. Try later.");
> > +        break;
> > +      case INFO_FORMAT_CHANGED:
> > +        // Not an error: will get this value when OMX output buffer is enabled.
> > +        CODEC_LOGD("decode dequeue OMX output buffer format change");
> 
> would this include resolution changes?

 When input resolution changed, the return value will be INFO_FORMAT_CHANGED followed by INFO_OUTPUT_BUFFERS_CHANGED. So the code in next case can handle it.

> 
> @@ +725,5 @@
> > +  if (aInputImage._length== 0 || !aInputImage._buffer) {
> > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > +  }
> > +
> > +  // 1st input carries SPS/PPS NALUs.
> 
> fix comment per previous review

 Will do in next version.

> 
> @@ +731,5 @@
> > +  if (firstFrame) {
> > +    mOMX = new WebrtcOMXDecoder(MEDIA_MIMETYPE_VIDEO_AVC);
> > +    // Get image width/height from parameter sets.
> > +    sp<ABuffer> paramSets = new ABuffer(aInputImage._buffer, aInputImage._length);
> > +    mOMX->ConfigureWithParamSets(paramSets);
> 
> This will fail if the first incoming packet isn't an SPS/PPS (and perhaps
> even if the order is swapped).

 Will change it to report error until SPS/PPS is found and configuration is successful.
> 
> @@ +678,5 @@
> > +
> > +  return WEBRTC_VIDEO_CODEC_OK;
> > +}
> > +
> > +// Note: stagefright doesn't handle frame rate change.
> 
> File a followup bug to reconfigure the codec (restart it if needed) on a
> frame rate change, since we need to do them for bitrate/load adaptation. 
> Put the bug number here.

 I'd like to understand what should encoder respond to frame rate change to make sure the implementation will be correct.
 I checked VP8 but failed to understand what it does. :( http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc?from=vp8_impl.cc#109
Flags: needinfo?(rjesup)
Blocks: 997110
(In reply to Randell Jesup [:jesup] from comment #81)
> Comment on attachment 8406157 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v2.2.
> 
> Review of attachment 8406157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, another issue that needs to be resolved.  What packetization mode is
> being used to send this data over RTP?  Mode 1?  
> 
> I think the encode/dequeue-encoded loop needs to be re-architected.  If it's
> single-large-NALU output (except for SPS/PPS NALs), then you can get away
> with this, probably, but it's not a good solution.  (mode 1 isn't
> necessarily bad or wrong to use in this context, but it has implications for
> the decoder side.  And even if it is used, assuming no dropped frames ever
> in the encoder is a dangerous assumption)
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +265,5 @@
> > +
> > +  status_t
> > +  FillInput(const webrtc::EncodedImage& aEncoded, bool aIsFirstFrame,
> > +            int64_t& aRenderTimeMs, webrtc::DecodedImageCallback* aCallback,
> > +            int64_t* aInputTimeUs)
> 
> Why bother having aInputTimeUs since the returned value is thrown away?
> 
> @@ +528,5 @@
> > +    }
> > +
> > +    // Tell base class not to pop input for SPS/PPS NALUs because they don't
> > +    // have corresponding input.
> > +    return !isParamSets;
> 
> Is the OMX encoder set to "generate a single large NALU" mode, with the
> packetization code set to fragment (H.264 SDP packetization-mode 1,
> Non-Interleaved mode)?  Otherwise, a single input frame may (usually will)
> generate multiple output NALUs which are sized to fit into a single packet
> (mode 0, Single NAL mode).
> 
> Perhaps it would be better to use the timestamp values coming out of the
> encoder, and either match/drop input packets by comparing to the output
> timestamps, or do a reset on resolution changes and cache width/height.
> 
> Also, it seems possible or likely that the encoder might decide to drop an
> input frame if it was temporarily far over bitrate (such as following an
> IDR), so relying on 1:1 input/output is not a good idea.  Perhaps the
> encoder doesn't in this setting - but it's bad to assume it can't.

 Follow-up bug 997110 is filed to address this properly after further study on OMX encoder behavior.
- convinced! Rewrite output drain to use monitor rather than repeatedly self-dispatching
- stop using AtomicRefCounted<>
- in Decode(), throw inputs away and report error until SPS/PPS is found and configuration is successful.
- address other comments
Attachment #8406157 - Attachment is obsolete: true
Attachment #8406157 - Flags: review?(ekr)
Attachment #8407463 - Flags: review?(rjesup)
If you want to test with OpenH264 peer, please check bug 996379 attachment 8407465 [details] [diff] [review].
Comment on attachment 8407463 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v3.

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

r- for now for the mOMX release question, and some cleanup needed (and maybe work to split SPS and PPS)

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +166,5 @@
> +  {
> +    MOZ_ASSERT(mThread);
> +
> +    while (true) {
> +      MonitorAutoLock lock(mMonitor);

Move this outside "while (true)"

We should hold this except while in Wait() of course, and when in DrainOutput().  We don't need to unlock/relock it on each loop as well

@@ +170,5 @@
> +      MonitorAutoLock lock(mMonitor);
> +      if (mEnding) {
> +        ALOGE("Ending OMXOutputDrain");
> +        // Stop draining.
> +        break;

move the mEnding check to after the if (...) { ... lock.Wait(); } block

@@ +179,5 @@
> +        lock.Wait();
> +      }
> +      EncodedFrame frame = mInputFrames.front();
> +      bool shouldPop = false;
> +      {

Add "if (frame)"

@@ +549,5 @@
> +    encoded.capture_time_ms_ = aInputFrame.mRenderTimeMs;
> +    encoded._length = output.Length();
> +    encoded._buffer = output.Elements();
> +    encoded._completeFrame = true;
> +    bool isParamSets = (flags & MediaCodec::BUFFER_FLAG_CODECCONFIG);

question provoked by the patch for bug 996379 - does it return one blob with two NALUs (SPS & PPS) and the the iframe?  Or does it return SPS, then return PPS, then return the iframe?

@@ +556,5 @@
> +
> +    if (mCallback) {
> +      mCallback->Encoded(encoded, nullptr, nullptr);
> +    }
> +

Add comment referencing bug 997110 and that we're assuming (for now) one frame in -> either one encoded frame, or SPS, PPS, and then an encoded frame.

@@ +708,5 @@
> +
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +// Note: stagefright doesn't handle frame rate change.

File a bug for this and give bug # here

@@ +757,5 @@
> +  }
> +
> +  ALOGE("WebrtcOMXH264VideoDecoder:%p will decode", this);
> +
> +  bool configured = mOMX;

configured = (mOMX != nullptr);
or 
configured = !!mOMX;

@@ +800,5 @@
> +WebrtcOMXH264VideoDecoder::Release()
> +{
> +  CODEC_LOGD("WebrtcOMXH264VideoDecoder:%p will be released", this);
> +
> +  mOMX = nullptr;

This is a raw ptr it appears, so I assume this is expected to just from the OMX structure on the floor, and not free it or make any calls to get rid of it?
Attachment #8407463 - Flags: review?(rjesup) → review-
Comment on attachment 8406116 [details] [diff] [review]
Part 2: Support 'handle-using' video frames for WebRTC on B2G.

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

This still seems to be doing the wrong thing in terms of abstracting the handle.

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ +45,5 @@
>  };
>  
> +// Give handle in video frame delivered from WebRTC a typename. It should be a
> +// webrtc::NativeHandle* that contains layer::Image*.
> +typedef void *VideoFrameHandle;

This isn't quite what I had in mind.

Is there some way for this to be a class that contains a layer::Image instead?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1073,5 @@
>    {
> +    // |handle| should contains layers::Image* if available.
> +    webrtc::NativeHandle* nh = static_cast<webrtc::NativeHandle*>(handle);
> +    mRenderer->RenderVideoFrame(buffer, buffer_size, time_stamp, render_time,
> +                                nh ? nh->GetHandle() : nullptr);

Please convert this away from a void * here.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +643,5 @@
>      virtual void RenderVideoFrame(const unsigned char* buffer,
>                                    unsigned int buffer_size,
>                                    uint32_t time_stamp,
> +                                  int64_t render_time,
> +                                  VideoFrameHandle handle) {

Can this be a ref?

@@ +648,3 @@
>        pipeline_->listener_->RenderVideoFrame(buffer, buffer_size, time_stamp,
> +                                             render_time,
> +                                             static_cast<layers::Image*>(handle));

And then this would do GetImage().

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +356,5 @@
>    void RenderVideoFrame(const unsigned char* buffer,
>                          unsigned int buffer_size,
>                          uint32_t time_stamp,
> +                        int64_t render_time,
> +                        mozilla::VideoFrameHandle handle)

Can this be a reference, since the handle is a ptr?
Attachment #8406116 - Flags: review?(ekr) → review-
Comment on attachment 8406122 [details] [diff] [review]
Part 5: Register H.264 external codec for B2G v1.1.

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2128,5 @@
> +  // TODO: bug 995884 to support H.264 in WebRTC.org code.
> +  if (config->mName != "I420") {
> +    // Do nothing for non-I420 config.
> +    return 0;
> +  }

This should return an error.

@@ +2148,5 @@
> +  }
> +#endif
> +  if (err != kMediaConduitNoError) {
> +    return VCM_ERROR;
> +  }

Why not just rewrite all these with early returns if things fail rather
than setting error and then checking it?
Attachment #8406122 - Flags: review?(ekr) → review-
(In reply to Randell Jesup [:jesup] from comment #87)
> Comment on attachment 8407463 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v3.
> 
> Review of attachment 8407463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for now for the mOMX release question, and some cleanup needed (and maybe
> work to split SPS and PPS)
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +549,5 @@
> > +    encoded.capture_time_ms_ = aInputFrame.mRenderTimeMs;
> > +    encoded._length = output.Length();
> > +    encoded._buffer = output.Elements();
> > +    encoded._completeFrame = true;
> > +    bool isParamSets = (flags & MediaCodec::BUFFER_FLAG_CODECCONFIG);
> 
> question provoked by the patch for bug 996379 - does it return one blob with
> two NALUs (SPS & PPS) and the the iframe?  Or does it return SPS, then
> return PPS, then return the iframe?

 It will be  SPS + PPS + iframe in one blob.

> 
> @@ +800,5 @@
> > +WebrtcOMXH264VideoDecoder::Release()
> > +{
> > +  CODEC_LOGD("WebrtcOMXH264VideoDecoder:%p will be released", this);
> > +
> > +  mOMX = nullptr;
> 
> This is a raw ptr it appears, so I assume this is expected to just from the
> OMX structure on the floor, and not free it or make any calls to get rid of
> it?

 Actually it's a RefPtr. See WebrtcOMXH264VideoCodec.h#160.

>
 
 All others will be fixed in next version.
Comment on attachment 8407463 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v3.

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

more defensive code suggestion

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +114,5 @@
> +{
> +  uint32_t mWidth;
> +  uint32_t mHeight;
> +  uint32_t mTimestamp;
> +  int64_t mRenderTimeMs;

bool isValid() const
{
  return (mWidth > 0) && (mHeight > 0);
}

@@ +180,5 @@
> +      }
> +      EncodedFrame frame = mInputFrames.front();
> +      bool shouldPop = false;
> +      {
> +        // Release monitor while draining because it's blocking.

MOZ_ASSERT(frame.isValid());
(In reply to Randell Jesup [:jesup] from comment #87)
> Comment on attachment 8407463 [details] [diff] [review]
> Part 4: Add external H.264 HW video codec implementation for B2G v3.
> 
> Review of attachment 8407463 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +179,5 @@
> > +        lock.Wait();
> > +      }
> > +      EncodedFrame frame = mInputFrames.front();
> > +      bool shouldPop = false;
> > +      {
> 
> Add "if (frame)"

 Since we wait when queue is empty, I don't think it's necessary to check. 
 Plus, frame is a local object, not a pointer.
Attached patch bug-911046-p4-v3.1.patch (obsolete) — Splinter Review
Address r- comments.
Attachment #8407463 - Attachment is obsolete: true
Attachment #8407999 - Flags: review?(rjesup)
Comment on attachment 8407999 [details] [diff] [review]
bug-911046-p4-v3.1.patch

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

packetization issues still aren't addressed at all. (sorry of comments here are slightly repetitive, I'm tired)

If this works (with no packetization code at either end, and NALU lengths limited to around 1200 max so they fit in a packet), then I would be ok to file a followup bug to packetize is according to RFC 6184 (tied in with the SDP changes needed).  However, note that it likely would NOT work with OpenH264 unless that also didn't packetize *and* it could process multiple NALUs out of a concatenated buffer.

Mode 0 is trivial; all you'd likely have to do (from comments you made) would be separate the SPS/PPS/first-iframe-NAL into separate Encoded() callbacks.  If you can't do mode 0, then mode 1 isn't hard (just separate SPS/PPS as before, and generate FU-A headers for fragmented large NALs).  I definitely recommend Mode 0 to start.

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +181,5 @@
> +        ALOGE("Ending OMXOutputDrain");
> +        // Stop draining.
> +        break;
> +      }
> +

ok, so no if (frame) -
Add MOZ_ASSERT(!mInputFrames.empty());

(if you hadn't moved "if (mEnding)" to after the wait, bad things would happen if you did mEnding = true; lock.NotifyAll() to kill the thread.  It's safe now (since the order is now correct), but the assert is worthwhile as a doublecheck against future code changes).

@@ +559,5 @@
> +    encoded._frameType = (isParamSets || (flags & MediaCodec::BUFFER_FLAG_SYNCFRAME)) ?
> +                         webrtc::kKeyFrame : webrtc::kDeltaFrame;
> +
> +    if (mCallback) {
> +      mCallback->Encoded(encoded, nullptr, nullptr);

Previous comment wasn't addressed:  "Add comment referencing bug 997110 and that we're assuming (for now) one frame in -> either one encoded frame, or SPS, PPS, and then an encoded frame."

@@ +564,5 @@
> +    }
> +
> +    // Tell base class not to pop input for SPS/PPS NALUs because they don't
> +    // have corresponding input.
> +    return !isParamSets;

You said "it will be SPS + PPS + iframe in one blob".  This would mean that the return here would be wrong, as the iframe is included.

However....

If they're in one blob, we have a real problem, since the overlying code needs the NALUs individually for packetization.  I suspect right now the code is punting on proper packetization, which would need to happen by adding code to rtp_sender_video.cc (need to add SendH264(), similar to SendVP8(), to do the packetization according to RFC 6184 and the negotiated parameters).  Alternatively, the packetization could happen here, in which case it should output compliant payloads.  I don't recommend this in general; as a kludge, if you're using mode 0 (the default - "Single NAL" mode), then you can output one NALU at a time from here (SPS, then PPS, then iframe NALs (likely several per iframe).  This requires telling the encoder not to use NALs larger than XXXX bytes (typically around 1400-1500, but actually depends on path-MTU).

If you can't tell it "max XXXX bytes per NAL", then you have to use mode 1 (non-interleaved mode), and add code here or in rtp_sender_video.cc (preferred) to do mode-1 packetization - use plain NALs if < 1 packet, FU-A's (fragmentation) if > 1 packet, and (optionally) combined small NALUs (SPS, PPS) in STAP-A aggregation packets.  This also means you must negotiate mode 1 (only) in the SDP, and fail if the other side doesn't do mode 1.  Note mode 0 is the most common in videophones and the list, and is the default.

@@ +687,5 @@
> +
> +  if (mOMX) {
> +    delete mOMX;
> +    mOMX = nullptr;
> +  }

So if mOMX in the decoder is a RefPtr, perhaps mOMX in the encoder should be too.  It's confusing to have one be raw, and the other be RefPtr.  This would become:
  mOMX = nullptr;
Attachment #8407999 - Flags: review?(rjesup) → review-
Mode 0(Single NAL unit mode)
1. WebrtcOMXH264VideoCodec need to know NAL size upper bound.
2. WebrtcOMXH264VideoCodec need to ask MediaCodec generate NAL less then upper bound.

Mode 1(Non-interleaved mode)
1. Support STAP-A type in RTP in rtp_sender_video.cc is required for this mode.
2. Negotiate mode 1 in SDP. If fail to setup this kind of channel, fallback to mode 0(Single NAL unit mode)

Mode 0 is must have since we can not make sure the other peer accept mode 1. As a result, we should support mode 0 here and treat mode 1 as following optimization(file another bug for mode 1)
(In reply to C.J. Ku[:CJKu] from comment #95)
> Mode 0(Single NAL unit mode)
> 1. WebrtcOMXH264VideoCodec need to know NAL size upper bound.
> 2. WebrtcOMXH264VideoCodec need to ask MediaCodec generate NAL less then
> upper bound.
This is a question mark. John, can we set up NAL size via MediaCodec public API?
Looks like QCOM encoder generates 1 NALU per frame in one output blob (except for SPS/PPS: they're combined in one output). And the size is usually much larger than RTP packet size.
(In reply to C.J. Ku[:CJKu] from comment #96)
> (In reply to C.J. Ku[:CJKu] from comment #95)
> > Mode 0(Single NAL unit mode)
> > 1. WebrtcOMXH264VideoCodec need to know NAL size upper bound.
> > 2. WebrtcOMXH264VideoCodec need to ask MediaCodec generate NAL less then
> > upper bound.
> This is a question mark. John, can we set up NAL size via MediaCodec public
> API?

 Khronos OMX IL API seems to have a config structure for NAL size: '4.3.31 OMX_VIDEO_CONFIG_NALSIZE' in spec v1.2.0 (http://www.khronos.org/registry/omxil/specs/OpenMAX_IL_1_2_0_Specification.pdf)
 But I found no appearance of the usage of that structure in QCOM video encoder or stagefright in B2G code base.
Hope this is getting closer to what you have in mind. :)
Attachment #8406116 - Attachment is obsolete: true
Attachment #8408355 - Flags: review?(ekr)
Address r- comments.
Attachment #8406122 - Attachment is obsolete: true
Attachment #8408357 - Flags: review?(ekr)
(In reply to John Lin[:jolin][:jhlin] from comment #98)
> > > Mode 0(Single NAL unit mode)
> > > 1. WebrtcOMXH264VideoCodec need to know NAL size upper bound.
> > > 2. WebrtcOMXH264VideoCodec need to ask MediaCodec generate NAL less then
> > > upper bound.
> > This is a question mark. John, can we set up NAL size via MediaCodec public
> > API?
> 
>  Khronos OMX IL API seems to have a config structure for NAL size: '4.3.31
> OMX_VIDEO_CONFIG_NALSIZE' in spec v1.2.0
> (http://www.khronos.org/registry/omxil/specs/OpenMAX_IL_1_2_0_Specification.
> pdf)
>  But I found no appearance of the usage of that structure in QCOM video
> encoder or stagefright in B2G code base.

This may be a significant problem; likely the reason you didn't find them is that the HW codecs & interfaces are targeted for media recording aka Video capture to SD.

If we can't get mode 0 support, we'll need to support only mode 1 for now and fall back to VP8 (or perhaps OpenH264) if the other side requires mode 0.   Talking to ourselves (and OpenH264 in FF) we should be ok; and we can lean on Chrome to support mode 1 when they support H.264.  But any sort of interop session or connection to a H.264 server may have problems.

We need to verify if all the devices we care about support max-nal-size APIs and know that it works.  In the meantime, it probably makes sense to work on mode 1 support anyways.  This will require a fragmenter in rtp_sender_video.cc to generate FU-A's (and maybe STAP-A's for SPS/PPS), and a reassembler and STAP-A disassembler in the rtp reception code.  These are relatively simple to do (I've written them in the past).  Also we need to make sure that the OMX code splits SPS and PPS into separate Encoded() callbacks.
Comment on attachment 8408355 [details] [diff] [review]
Part 2: Support 'handle-using' video frames for WebRTC on B2G v1.2.

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

::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
@@ +52,5 @@
> + */
> +class ImageHandle
> +{
> +public:
> +    ImageHandle(layers::Image* image) : mImage(image) {}

Fix indent here and below.

@@ +54,5 @@
> +{
> +public:
> +    ImageHandle(layers::Image* image) : mImage(image) {}
> +
> +    layers::Image* GetImage() const { return mImage; }

Having an accessor for the raw pointer seems kinda scary here. Is there a reason not to return RefPtr?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +1441,5 @@
> +#ifdef WEBRTC_GONK
> +  else {
> +    // Decoder produced video frame that can be appended to the track directly.
> +    MOZ_ASSERT(video_image);
> +    image_ = video_image;

Since this is making a strong reference this would be much clearer if there were RefPtr being passed rather than a raw ptr.

Suggest having GetImage() return a const RefPtr<Image>
Attachment #8408355 - Flags: review?(ekr) → review-
Comment on attachment 8408357 [details] [diff] [review]
Part 5: Register H.264 external codec for B2G v1.2.

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2146,5 @@
> +    }
> +  }
> +#endif
> +
> +  return 0;

Please put some sort of assert here since this is NOTREACHED.
Attachment #8408357 - Flags: review?(ekr) → review+
Raw ptr -> RefPtr.
Attachment #8408355 - Attachment is obsolete: true
Attachment #8408746 - Flags: review?(ekr)
nit fix and carry the r+ from :ekr.
Attachment #8408357 - Attachment is obsolete: true
Attachment #8408747 - Flags: review+
(In reply to John Lin[:jolin][:jhlin] from comment #83)
> > 
> > @@ +678,5 @@
> > > +
> > > +  return WEBRTC_VIDEO_CODEC_OK;
> > > +}
> > > +
> > > +// Note: stagefright doesn't handle frame rate change.
> > 
> > File a followup bug to reconfigure the codec (restart it if needed) on a
> > frame rate change, since we need to do them for bitrate/load adaptation. 
> > Put the bug number here.
> 
>  I'd like to understand what should encoder respond to frame rate change to
> make sure the implementation will be correct.
>  I checked VP8 but failed to understand what it does. :(
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/codecs/vp8/vp8_impl.cc?from=vp8_impl.cc#109

That's complicated by VP8's support for temporal layers (multi-layered encoding so you can drop packets and just reduce the frame rate).

This command to the codec says "The input frame rate is nominally X".  For example, if we're badly over-bandwidth, we might drop the frame rate to 20fps from 30 and we *should* inform the encoder of this (the encoder could infer the frame rate by watching input timestamps, but it's easier to be told).  Or (especially) if we're overloaded CPU-wide, we might drop the frame rate.

However, I note from the code that currently it appears to not update this value in mid-call.  We do want to support that, since we're looking at hooking up the dynamic frame rate option.  So this isn't critical to do for the first cut.  File a followup and put the number here
Flags: needinfo?(rjesup)
Blocks: 998220
No longer blocks: 998220
Blocks: 998220
Comment on attachment 8408746 [details] [diff] [review]
Part 2: Support 'handle-using' video frames for WebRTC on B2G v1.3.

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

lgtm
Attachment #8408746 - Flags: review?(ekr) → review+
- Add a function to get SPS/PPS NALUs from AVC/H.264 encoder for WebRTC encoder to prepend them to every I-frame.
- Rewrite GenerateAVCDescriptorBlob() to parameter sets NALUs parsed by OMX encoder rather than do the parsing ourselves.
Attachment #8400384 - Attachment is obsolete: true
Attachment #8409508 - Flags: review?(rjesup)
Address r- comments
- make OMXVideoEncoder ref-counted
- WebRTC encoder now output individual NALU to encoded callback for RTP sender to support mode-0 and STAP-A in mode-1
- prepend SPS/PPS to I-frames
Attachment #8409508 - Attachment is obsolete: true
Attachment #8409508 - Flags: review?(rjesup)
Attachment #8409510 - Flags: review?(rjesup)
Attachment #8409510 - Flags: review?(ekr)
Attachment #8409508 - Attachment is obsolete: false
Attachment #8409508 - Flags: review?(rjesup)
Attachment #8407999 - Attachment is obsolete: true
Hi John,
if we are not intend to fix " [WIP] Changes to stagefright to make other patches work." in this bug, make sure we have another issue file of targeting this problem, and that bug need to be a blocker of 1.5(2.0)
Comment on attachment 8409510 [details] [diff] [review]
Part 4: Add external H.264 HW video codec implementation for B2G v4.

Thanks!  That should work much better.
Attachment #8409510 - Flags: review?(rjesup) → review+
Comment on attachment 8409508 [details] [diff] [review]
Part 3: Add support to WebRTC in OMXCodecWrapper v2.

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

Overall a lot simpler!

::: content/media/omx/OMXCodecDescriptorUtil.cpp
@@ +85,5 @@
> +    // 2 bytes length value.
> +    uint8_t size[] = {
> +      (mSize & 0xFF00) >> 8, // MSB.
> +      mSize & 0x00FF,        // LSB.
> +    };

manual htons(mSize), but that's not a problem
Attachment #8409508 - Flags: review?(rjesup) → review+
Status: NEW → ASSIGNED
Hi John, CJ - I talked to Randell and Ekr, and we think it makes sense to land this feature pref'd off (disabled) to start.  Once we get the worst of the known issues resolved, we can switch the pref to "enabled".  

FYI: To change the pref without doing a new build, you can create a .js file (e.g. h264.js) with the H.264 pref set to true and drop it on the phone in /system/b2g/default/prefs/h264.js
Flags: needinfo?(jolin)
Flags: needinfo?(cku)
Obsolete the WIP changes to stagefright that was needed to get graphic buffers from OMX decoder.
Attachment #8392645 - Attachment is obsolete: true
Attachment #8410015 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8410015 - Flags: feedback?(bwu)
Flags: needinfo?(jolin)
Attachment #8410015 - Attachment description: [WIP] Use gonk native window callback to get decoder output graphic buffer. → [WIP] Part 4.5: Use gonk native window callback to get decoder output graphic buffer.
Hi Maire, agree. Land patches with pref off is exactly we will do
Flags: needinfo?(cku)
Updated for checking-in.
Attachment #8406121 - Attachment is obsolete: true
Attachment #8410061 - Flags: review+
Update for checking-in.
Attachment #8408746 - Attachment is obsolete: true
Attachment #8410062 - Flags: review+
Update for checking-in.
Attachment #8409508 - Attachment is obsolete: true
Attachment #8410063 - Flags: review+
Update for checking-in.
Attachment #8409510 - Attachment is obsolete: true
Attachment #8409510 - Flags: review?(ekr)
Attachment #8410064 - Flags: review+
Update for checking-in.
Attachment #8408747 - Attachment is obsolete: true
Attachment #8410065 - Flags: review+
Update for checking-in.
Attachment #8399309 - Attachment is obsolete: true
Attachment #8410066 - Flags: review+
Try result: https://tbpl.mozilla.org/?tree=Try&rev=389aae75182d

Patches need to check-in: Part 1 ~ 6.
This is some really great work here. Thanks!
Whiteboard: [ft:multimedia-platform][webrtc]
Target Milestone: --- → 1.4 S6 (25apr)
Hi Carsten,
 Only need to check in Part 1 ~ Part 6.
 Please back out https://hg.mozilla.org/integration/mozilla-inbound/rev/4672ae9dd0c8
 Thanks a lot.
Flags: needinfo?(cbook)
Corrected the pref value to the agreed value of false under rs=moduleowner; no backout needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a26bb25da8
Flags: needinfo?(cbook)
Comment on attachment 8410065 [details] [diff] [review]
Part 5: Register H.264 external codec for B2G.

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

Note: this landed as r=jesup,ekr but it was only r+'d by ekr 

No immediate action required.

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +82,5 @@
> +    // supported video codec in |webrtc::ViECodecImpl::CodecValid()| and will
> +    // reject registration of those not in it.
> +    // TODO: bug 995884 to support H.264 in WebRTC.org code.
> +    if (mName == "H264_P0")
> +      mName = "I420";

brace if body

@@ +103,5 @@
> +    // supported video codec in |webrtc::ViECodecImpl::CodecValid()| and will
> +    // reject registration of those not in it.
> +    // TODO: bug 995884 to support H.264 in WebRTC.org code.
> +    if (mName == "H264_P0")
> +      mName = "I420";

brace if body

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2417,5 @@
> +    if (!conduit)
> +      return VCM_ERROR;
> +
> +    if (vcmEnsureExternalCodec(conduit, config_raw, true))
> +      return VCM_ERROR;

I would say to brace these 3 ifs, but the rest of the file doesn't, so we should keep local style, especially as this has landed.
Attachment #8410015 - Flags: feedback?(chung)
Comment on attachment 8410015 [details] [diff] [review]
[WIP] Part 4.5: Use gonk native window callback to get decoder output graphic buffer.

Looks good. One comment. GonkNativeWindowClient and Surface are same things. If Surface is used for GonkNativeWindow, GonkNativeWindowClient is not necessary.
Attachment #8410015 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Part 4 breaks Flatfish build:

media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp: In member function 'android::status_t mozilla::WebrtcOMXDecoder::ConfigureWithParamSets(const android::sp<android::MetaData>&)':
media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp:295:52: error: 'class android::GonkNativeWindowClient' has no member named 'getIGraphicBufferProducer'
media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp: In member function 'webrtc::I420VideoFrame* mozilla::WebrtcOMXDecoder::GenerateVideoFrame(const mozilla::EncodedFrame&, uint32_t, const android::sp<android::ABuffer>&)':
media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp:482:65: error: invalid static_cast from type 'android::RefBase*' to type 'android::GraphicBuffer*'
(In reply to Sotaro Ikeda [:sotaro] from comment #129)
> Comment on attachment 8410015 [details] [diff] [review]
> [WIP] Part 4.5: Use gonk native window callback to get decoder output
> graphic buffer.
> 
> Looks good. One comment. GonkNativeWindowClient and Surface are same things.
> If Surface is used for GonkNativeWindow, GonkNativeWindowClient is not
> necessary.
 Cool! Thanks for the info.
- obsolete the patch to stagefright for getting decoder output
- remove usage of GonkNativeWindowClient as sotaro suggested in comment 129
Attachment #8410015 - Attachment is obsolete: true
Attachment #8410015 - Flags: feedback?(chung)
Attachment #8410015 - Flags: feedback?(bwu)
Attachment #8411669 - Flags: review?(rjesup)
Comment on attachment 8411669 [details] [diff] [review]
Use gonk native window callback to get decoder output graphic buffer.

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

r+, but you need to have someone who knows the graphics buffers look at it too.

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +508,5 @@
>  
>    RefPtr<OutputDrain> mOutputDrain;
> +  Mutex mDecodedFrameLock; // To protect mDecodedFrames.
> +  std::queue<EncodedFrame> mDecodedFrames;
> +  webrtc::DecodedImageCallback* mCallback;

separate mCallback from the stuff protected by the lock - blank line, or move mCallback above lock and add a blank line before lock
Attachment #8411669 - Flags: review?(rjesup) → review+
There appears to be a conflict between the encoder, which strips the NAL 
headers:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp#525


However, the decoder looks for the NAL headers:
http://osxr.org/android/source/frameworks/av/media/libstagefright/avc_utils.cpp#0247

And if this fails, then it seems like the decoder will never
parse the parameters and therefore will refuse to decode real
frames:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp#525

I found this in my standalone harness. When I ran it by John Lin
and he seems to agree that this is likely to be a problem in the
live system.
Try result: https://tbpl.mozilla.org/?tree=Try&rev=880e792684d0(In reply to Randell Jesup [:jesup] from comment #133)
> Comment on attachment 8411669 [details] [diff] [review]
> Use gonk native window callback to get decoder output graphic buffer.
> 
> Review of attachment 8411669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, but you need to have someone who knows the graphics buffers look at it
> too.

 Sotaro already helped look at the patch in comment 131.
- Address nit and carry r+ from jesup.
- Try result: https://tbpl.mozilla.org/?tree=Try&rev=880e792684d0
Attachment #8411669 - Attachment is obsolete: true
Attachment #8413512 - Flags: review+
Please help land *only* attachment 8413512 [details] [diff] [review] "Use gonk native window callback to get decoder output graphic buffer".
(In reply to Eric Rescorla (:ekr) from comment #134)
> There appears to be a conflict between the encoder, which strips the NAL 
> headers:
> 
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> media-conduit/WebrtcOMXH264VideoCodec.cpp#525
> 
> 
> However, the decoder looks for the NAL headers:
> http://osxr.org/android/source/frameworks/av/media/libstagefright/avc_utils.
> cpp#0247
> 
> And if this fails, then it seems like the decoder will never
> parse the parameters and therefore will refuse to decode real
> frames:
> 
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> media-conduit/WebrtcOMXH264VideoCodec.cpp#525
> 
> I found this in my standalone harness. When I ran it by John Lin
> and he seems to agree that this is likely to be a problem in the
> live system.

bug 1002402 filed to fix this.
https://hg.mozilla.org/mozilla-central/rev/1447c1d13eb0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Depends on: 1005418
feature-b2g: --- → 2.0
You need to log in before you can comment on or make changes to this bug.