969395 - [WebRTC] Support hardware accelerated vp8 encoding & decoding for WebRTC

Status: NEW

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

Description

[qiang lu]

This bug is used for tracking the implementation of an experimental feature on Fennec which try to introduce VP8 hardware acceleration into webrtc stack
basically, it is implemented according to John Lin's WIP branch for Firefox OS. (see https://github.com/jhlin/mozilla-central/tree/WIP-jb/media/webrtc/signaling/src/media-conduit)
It should work on both x86 and arm based devices.

Comment 1

[qiang lu]

Attachment: B969395_stub_library.v01.patch

This is first patch for enabling vp8 hw codec on fennec.

Add three stub library for accessing VP8 HW codec through android native mediacodec interface by webrtc module.

Comment 2

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v01.patch

enabling VP8 HW codec in webrtc module on fennec based on android native mediacodec interface.
This patch is implemented based on john Lin's WIP branch:
https://github.com/jhlin/mozilla-central/tree/42667e46ac02d3228dbc43a7d6c624f677321c64/media/webrtc/signaling/src/media-conduit

Note, i tested it on intel merrfield pr2 development board which support HW vp8 encode&decode. 
And i am trying to test it on google nexus 5.

Comment 3

[u459114]

The implementation of this bug is relative to bug 911046

Comment 4

[Gian-Carlo Pascutto [:gcp]]

I'll try to review this ASAP but I'm on a workweek.

Comment 5

[Eric Rescorla (:ekr)]

(In reply to C.J. Ku[:CJKu] from comment #3)
> The implementation of this bug is relative to bug 911046

Yeah, let's hold this patch until we have 911046 a little more advanced.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8373230 [details] [diff] [review]
B969395_stub_library.v01.patch

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

Deferring review to someone more familiar with omxplugin machinery.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8373244 [details] [diff] [review]
B969395_webrtc_hw_codec.v01.patch

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

Some basic comments, this is clearly WIP. I'm asking cpeterson and jesup to take a look at it as well.

::: media/webrtc/signaling/signaling.gyp
@@ +182,5 @@
>          'HAVE_UINT8_T=1',
>          'HAVE_UINT16_T=1',
>          'HAVE_UINT32_T=1',
>          'HAVE_UINT64_T=1',
> +        'HAVE_SYS_UIO_H',

Can't find why this is now required?

@@ +228,5 @@
> +
> +		    'cflags': [
> +              '-Wno-multichar',
> +          ],
> +          

nit: careful with whitespace, extra whitespace is present in several places in the patches, which messes up the code indentation as well

@@ +235,5 @@
> +              '-lstagefright',
> +              '-lstagefright_foundation',
> +              '-lutils',
> +            ],
> +          },

Hmm, don't we avoid doing this now because it's not compatible between Android versions? This might work for Firefox OS but I don't think it works on Android.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +533,5 @@
> +  CSFLogDebug(logTag, "%s, Before ExtVideoCodec codec %s(%d)", __FUNCTION__,
> +                        video_codec.plName, video_codec.plType);
> +
> +  // FIXME
> +  if (video_codec.plType == 120) {

What's 120? Shouldn't hardcode the value.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +13,5 @@
>  #include "webrtc/common_types.h"
> +
> +#ifdef FF
> +#undef FF
> +#endif

Hmmm?

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +10,5 @@
> +
> +#include <mozilla/Scoped.h>
> +#include "VideoConduit.h"
> +#include "AudioConduit.h"
> +#include "../../../trunk/webrtc/video_engine/include/vie_external_codec.h"

The ../../../trunk parts here should be unneeded I believe it's part of the include path at this point. Also clean up the order/grouping of the includes please.

@@ +31,5 @@
> +using namespace android;
> +
> +#define EXT_VIDEO_PLAYLOAD_NAME "VP8"
> +#define EXT_VIDEO_FRAME_WIDTH 640 
> +#define EXT_VIDEO_FRAME_HEIGHT 480

These aren't truly constants are they?

@@ +241,5 @@
> +  memcpy(dstY, inputImage.buffer(webrtc::kYPlane), sizeY);
> +  for (int i = 0; i < sizeUV;
> +      i++, dstUV++, u++, v++) {
> +    *dstUV = (*v << 8) + *u;
> +  }

Maybe throw this in a function?

@@ +464,5 @@
> +        *dstV++ = *uv++;
> +      }
> +      uv += padding;
> +    }
> +  }

What conversion is this doing exactly? (comment)

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h
@@ +23,5 @@
> +
> +#include "MediaConduitInterface.h"
> +#include "AudioConduit.h"
> +#include "VideoConduit.h"
> +#include "../../../trunk/webrtc/modules/video_coding/codecs/interface/video_codec_interface.h"

Same remark as the cpp file.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8373244 [details] [diff] [review]
B969395_webrtc_hw_codec.v01.patch

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

This will be a cool feature! :) For your next round of reviews, you should ask :doublec (Chris Double) for review because he wrote most of our Android OMX code.

General comments:
• Check indentation.
• When long function signatures are wrapped, the new line should be indented so the parameter types line up.
• Trailing whitespace should be removed in Mozilla files. Third-party files (like the stagefright headers) should be left as-is.
• All overridden virtual functions should have an MOZ_OVERRIDE annotation: virtual void MyFunction() MOZ_OVERRIDE;
• You should try to use camelCased names like `videoCodec` in Mozilla code, unless you are interfacing with third-party code that uses "snake_case" names.
• Code that is commented out should probably be removed or have a FIXME comment with a Bugzilla bug number for follow-up work.
• Some functions are hardcoded to return WEBRTC_VIDEO_CODEC_OK even when calling OMX functions that return err != OK.
• Some hot functions like Encode() and Decode() are called for every video frame, but they call PR_IntervalNow() multiple times just for debug log messages. PR_IntervalNow() might be unnecessarily expensive on some platforms. Can we #ifdef the PR_IntervalNow() code only for debug builds or if some logging macro is #defined?

::: media/webrtc/signaling/signaling.gyp
@@ +228,5 @@
> +
> +		    'cflags': [
> +              '-Wno-multichar',
> +          ],
> +          

moz.build files are a restricted subset of Python, so indentation can be important.

::: media/webrtc/signaling/src/media-conduit/ExtVideoCodec.h
@@ +6,5 @@
> +#define ExtVideoCodec_h__
> +
> +#include "MediaConduitInterface.h"
> +
> +#define EXT_VIDEO_PAYLOAD_TYPE 100

Why is EXT_VIDEO_PAYLOAD_TYPE needed in a public header? This macro does not seem to be used anywhere.

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +5,5 @@
> +#include "CSFLog.h"
> +#include "nspr.h"
> +
> +#include <iostream>
> +//#include "omx_data_dump.h"

Remove commented code.

@@ +12,5 @@
> +#include "VideoConduit.h"
> +#include "AudioConduit.h"
> +#include "../../../trunk/webrtc/video_engine/include/vie_external_codec.h"
> +#include "runnable_utils.h"
> +#include "WebrtcExtVideoCodec.h"

WebrtcExtVideoCodec.h should be the first header #included in WebrtcExtVideoCodec.cpp. This ensures WebrtcExtVideoCodec.h is self-sufficient, including all the headers and definitions it needs.

@@ +25,5 @@
> +#include <IOMX.h>
> +#include <OMX_VideoExt.h>
> +#include <stagefright/MediaCodec.h>
> +#include <stagefright/MediaDefs.h>
> +#include <stagefright/MediaErrors.h>

These #includes should be alphabetized and grouped like: WebrtcExtVideoCodec.h, then a blank line, "mozilla/whatever.h" headers, a blank line, then system and non-Mozilla headers, a blank line, then miscellaneous Mozilla or webrtc headers.

https://hg.mozilla.org/mozilla-central/file/5f1d4098333f/mfbt/STYLE#l218

@@ +35,5 @@
> +#define EXT_VIDEO_FRAME_HEIGHT 480
> +#define EXT_VIDEO_MAX_FRAMERATE 30
> +#define DEQUEUE_BUFFER_TIMEOUT_US (100 * 1000ll) // 100ms
> +#define START_DEQUEUE_BUFFER_TIMEOUT_US (10 * DEQUEUE_BUFFER_TIMEOUT_US) // 1s
> +#define MEDIACODEC_VIDEO_MIME_VP8 "video/x-vnd.on2.vp8"

All these #defines should probably use real static const types.

@@ +38,5 @@
> +#define START_DEQUEUE_BUFFER_TIMEOUT_US (10 * DEQUEUE_BUFFER_TIMEOUT_US) // 1s
> +#define MEDIACODEC_VIDEO_MIME_VP8 "video/x-vnd.on2.vp8"
> +
> +#include <cstdio>
> +#include <utils/Log.h>

These #includes should be grouped with the others above, before any #defines.

@@ +54,5 @@
> + 
> +    looper_ = new ALooper;
> +    looper_->start();
> +    msg_ = new AMessage;
> +    omx_ = MediaCodec::CreateByType(looper_, mime, encoder);

Can MediaCodec::CreateByType() fail? Do we need to handle any errors here or check for null omx_ in other member functions?

@@ +59,5 @@
> +  }
> +
> +  virtual ~WebrtcOMX() {
> +	  msg_.clear();
> +    looper_.clear();

Fix indentation.

@@ +64,5 @@
> +    omx_.clear();
> +  }
> +
> +  status_t Configure(const sp<Surface>& nativeWindow,
> +    const sp<ICrypto>& crypto, uint32_t flags) {

When long function signatures are wrapped, the new line should be indented so the parameter types line up.

@@ +78,5 @@
> +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +
> +    status_t err = omx_->start();
> +	  CSFLogDebug(logTag, "WebrtcOMX::%s, omx_->start() return err = %d",
> +               __FUNCTION__, err);

Fix indentation.

@@ +99,5 @@
> +
> +  friend class WebrtcExtVideoEncoder;
> +  friend class WebrtcExtVideoDecoder;
> +
> +  sp<ALooper> looper_;

Make these member variables private.

@@ +118,5 @@
> +    callback_(nullptr),
> +    encoder_(nullptr) {
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +//  dataEncoderDump_ = new OMXDataDump();
> +//  dataEncoderDump_->Init("video/x-vnd.on2.vp8");

Remove commented code.

@@ +135,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);

Why is all this code commented out? It should be removed or have a FIXME comment with a Bugzilla bug number for follow-up work.

@@ +158,5 @@
> +    uint32_t maxPayloadSize) {
> +  max_payload_size_ = maxPayloadSize;
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +
> +  if (omxEnc == nullptr) { 

Just check `if (!omxEnc)`, not `== nullptr`.

@@ +160,5 @@
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +
> +  if (omxEnc == nullptr) { 
> +    // FIXME: implement proper lifecycle management
> +	  omxEnc = new WebrtcOMX("video/x-vnd.on2.vp8", true /* encoder */);

Replace hardcoded string with MEDIACODEC_VIDEO_MIME_VP8 (from above)?

@@ +164,5 @@
> +	  omxEnc = new WebrtcOMX("video/x-vnd.on2.vp8", true /* encoder */);
> +    // FIXME: use input parameters from webrtc negotiation
> +    webrtc::VideoCodec codec_inst;
> +    memset(&codec_inst, 0, sizeof(webrtc::VideoCodec));
> +    strncpy(codec_inst.plName, EXT_VIDEO_PLAYLOAD_NAME, 31);

Where does the magic number 31 come from? Should you use MOZ_ARRAY_LENGTY(EXT_VIDEO_PLAYLOAD_NAME)? EXT_VIDEO_PLAYLOAD_NAME is "VP8", so strncpy() is never going to copy 31 chars.

@@ +177,5 @@
> +
> +    VideoCodecSettings2AMessage(omxEnc->msg_, &codec_inst, MEDIACODEC_VIDEO_MIME_VP8, true /*encoder*/);
> +    
> +    omxEnc->Configure(nullptr, nullptr,
> +      MediaCodec::CONFIGURE_FLAG_ENCODE);

Can Configure() fail? Also, remove trailing whitespace and I don't think you need to line wrap here.

@@ +186,5 @@
> +  // TODO: eliminate extra pixel copy & color conversion
> +  size_t size = EXT_VIDEO_FRAME_WIDTH * EXT_VIDEO_FRAME_HEIGHT * 3 / 2;
> +  if (encoded_image_._size < size) {
> +    if (encoded_image_._buffer) {
> +      delete [] encoded_image_._buffer;

You can remove this null _buffer check because C++ allows you to safely call delete or delete[] on a null pointer.

@@ +383,5 @@
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +// TODO: eliminate global variable after implementing prpper lifecycle management code
> +static WebrtcOMX* omxDec = nullptr;

Why can't omxDec be a member variable of WebrtcExtVideoDecoder?

@@ +397,5 @@
> +    int32_t numberOfCores) {
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +  // FIXME: implement proper lifecycle management
> +  if (omxDec == nullptr) {
> +	omxDec  = new WebrtcOMX("video/x-vnd.on2.vp8", false /* decoder */);

Replace hardcoded string with MEDIACODEC_VIDEO_MIME_VP8 (from above)?

@@ +401,5 @@
> +	omxDec  = new WebrtcOMX("video/x-vnd.on2.vp8", false /* decoder */);
> +    // FIXME: use input parameters from webrtc negotiation
> +    webrtc::VideoCodec codec_inst;
> +    memset(&codec_inst, 0, sizeof(webrtc::VideoCodec));
> +    strncpy(codec_inst.plName, EXT_VIDEO_PLAYLOAD_NAME, 31);

Where does the magic number 31 come from? Should you use MOZ_ARRAY_LENGTY(EXT_VIDEO_PLAYLOAD_NAME)? EXT_VIDEO_PLAYLOAD_NAME is "VP8", so strncpy() is never going to copy 31 chars.

@@ +419,5 @@
> +               __FUNCTION__, err);
> +    if (err != OK) {
> +		CSFLogDebug(logTag,  "MediaCodecDecoderImpl::%s, decoder configure return err = %d",
> +               __FUNCTION__, err);
> +	}

Fix indentation.

@@ +424,5 @@
> +  }
> +  omxDec->started_ = false;
> +  decoder_ = omxDec;
> +
> +  return WEBRTC_VIDEO_CODEC_OK;

Why do we return WEBRTC_VIDEO_CODEC_OK even when omxDec->Configure() returns err != OK?

@@ +428,5 @@
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +void generateVideoFrame_sw(EncodedFrame* frame, const sp<ABuffer>& decoded,
> +  webrtc::I420VideoFrame* videoFrame) {

Make function static and capitalized. Fix wrapped line indentation and trailing whitespace.

@@ +469,5 @@
> +
> +  videoFrame->set_timestamp(frame->timestamp_);
> +}
> +
> +status_t feedOMXInput(WebrtcOMX* decoder, const sp<MediaCodec>& omx,

Make function static and capitalized.

@@ +504,5 @@
> +
> +  return err;
> +}
> +
> +status_t getOMXOutput(WebrtcOMX* decoder, const sp<MediaCodec>& omx,

Make function static and capitalized.

@@ +577,5 @@
> +  }
> +
> +  uint32_t time = PR_IntervalNow();
> +  WebrtcOMX* decoder = static_cast<WebrtcOMX*>(decoder_);
> +  if (!decoder->started_) {

Move `time = PR_IntervalNow()` instead the `if (!decoder->started_)` block. We don't want to call PR_IntervalNow() for *every* video frame when we only use it to log one debug message when calling start().

@@ +595,5 @@
> +    } while (err == INFO_OUTPUT_BUFFERS_CHANGED);
> +  }
> +
> +  CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() end", __FUNCTION__);
> +  return WEBRTC_VIDEO_CODEC_OK;

Why do we return WEBRTC_VIDEO_CODEC_OK when feedOMXInput() or getOMXOutput() might return errors?

::: media/webrtc/signaling/test/Makefile.in
@@ +159,5 @@
>  ifeq ($(OS_TARGET), Android)
>  LOCAL_INCLUDES +=  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/android/include
> +LIBS += \
> +	-L$(DEPTH)/media/omx-plugin/lib/kk/libutils \
> +  -lutils \

Fix -L indentation.

::: toolkit/library/Makefile.in
@@ +125,5 @@
>  
>  ifdef MOZ_WEBRTC
> +OS_LIBS += \
> +	-L$(DEPTH)/media/omx-plugin/lib/kk/libutils \
> +  -lutils \

Fix -L indentation.

::: toolkit/toolkit.mozbuild
@@ +79,5 @@
>  if CONFIG['MOZ_OMX_PLUGIN']:
>      add_tier_dir('platform', [
> +        'media/omx-plugin/lib/kk/libutils',
> +        'media/omx-plugin/lib/kk/libstagefright_foundation',
> +        'media/omx-plugin/lib/kk/libstagefright',

libstagefright_foundation should be listed after libstagefright.

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8373230 [details] [diff] [review]
B969395_stub_library.v01.patch

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

LGTM, I guess, but you should ask :doublec (Chris Double) for review because he wrote most of our Android OMX code.

Comment 10

[qiang lu]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #7)
> Comment on attachment 8373244 [details] [diff] [review]
> B969395_webrtc_hw_codec.v01.patch
> 
> Review of attachment 8373244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some basic comments, this is clearly WIP. I'm asking cpeterson and jesup to
> take a look at it as well.
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +182,5 @@
> >          'HAVE_UINT8_T=1',
> >          'HAVE_UINT16_T=1',
> >          'HAVE_UINT32_T=1',
> >          'HAVE_UINT64_T=1',
> > +        'HAVE_SYS_UIO_H',
> 
> Can't find why this is now required?
I need this flag to resolve redefinition of 'struct iovec' issue which is included by gui/Surface.h and defined in "media/omx-plugin/include/kk/cutils/log.h" and "android-ndk-r8e/platforms/android-9/arch-x86/usr/include/linux/socket.h"
see compile log below:

0:18.89 In file included from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/cutils/log.h:40:0,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/utils/Vector.h:24,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/binder/IBinder.h:23,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/binder/Binder.h:20,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/binder/IInterface.h:21,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/gui/IGraphicBufferProducer.h:26,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/gui/Surface.h:20,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:19:
 0:18.89 /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/cutils/uio.h:33:8: error: redefinition of 'struct iovec'
 0:18.89 In file included from /home/luq/android-ndk-r8e/platforms/android-9/arch-x86/usr/include/linux/socket.h:29:0,
 0:18.89                  from /home/luq/android-ndk-r8e/platforms/android-9/arch-x86/usr/include/sys/socket.h:33,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/objdir-droid-x86/dist/include/nspr/prinet.h:37,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/objdir-droid-x86/dist/include/nspr/nspr.h:17,
 0:18.89                  from /home/luq/Dev/firefox/firefox_for_android/x86_latest/src/media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:6:
 0:18.89 /home/luq/android-ndk-r8e/platforms/android-9/arch-x86/usr/include/linux/uio.h:18:8: error: previous definition of 'struct iovec'


> 
> @@ +228,5 @@
> > +
> > +		    'cflags': [
> > +              '-Wno-multichar',
> > +          ],
> > +          
> 
> nit: careful with whitespace, extra whitespace is present in several places
> in the patches, which messes up the code indentation as well

Done.

> 
> @@ +235,5 @@
> > +              '-lstagefright',
> > +              '-lstagefright_foundation',
> > +              '-lutils',
> > +            ],
> > +          },
> 
> Hmm, don't we avoid doing this now because it's not compatible between
> Android versions? This might work for Firefox OS but I don't think it works
> on Android.
> 
Done. remove it(i forgot delete it after i implemented the stub library), it is not used on Fennec build.

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +533,5 @@
> > +  CSFLogDebug(logTag, "%s, Before ExtVideoCodec codec %s(%d)", __FUNCTION__,
> > +                        video_codec.plName, video_codec.plType);
> > +
> > +  // FIXME
> > +  if (video_codec.plType == 120) {
> 
> What's 120? Shouldn't hardcode the value.

Remove this payload type check which is used for enable HW acceleration when incoming stream is VP8. Since official webrtc stack don't support other codec, this check can be removed safely.
And i fired another bug 973761 to track&implement an mechanism which can be used for enable/disable HW codec support on demand.

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +13,5 @@
> >  #include "webrtc/common_types.h"
> > +
> > +#ifdef FF
> > +#undef FF
> > +#endif
> 
> Hmmm?
Done. remove it.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +10,5 @@
> > +
> > +#include <mozilla/Scoped.h>
> > +#include "VideoConduit.h"
> > +#include "AudioConduit.h"
> > +#include "../../../trunk/webrtc/video_engine/include/vie_external_codec.h"
> 
> The ../../../trunk parts here should be unneeded I believe it's part of the
> include path at this point. Also clean up the order/grouping of the includes
> please.

Done.

> 
> @@ +31,5 @@
> > +using namespace android;
> > +
> > +#define EXT_VIDEO_PLAYLOAD_NAME "VP8"
> > +#define EXT_VIDEO_FRAME_WIDTH 640 
> > +#define EXT_VIDEO_FRAME_HEIGHT 480
> 
> These aren't truly constants are they?
Yes, they shouldn't be constants. I left them there for debuging only. The value of those variables should be passed from up layer(VideoConduit.cpp).

I will remove them and send another patch to review.

> 
> @@ +241,5 @@
> > +  memcpy(dstY, inputImage.buffer(webrtc::kYPlane), sizeY);
> > +  for (int i = 0; i < sizeUV;
> > +      i++, dstUV++, u++, v++) {
> > +    *dstUV = (*v << 8) + *u;
> > +  }
> 
> Maybe throw this in a function?

Done. move these code to a new function.

> 
> @@ +464,5 @@
> > +        *dstV++ = *uv++;
> > +      }
> > +      uv += padding;
> > +    }
> > +  }
> 
> What conversion is this doing exactly? (comment)

Done. Add comments to explain it.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h
> @@ +23,5 @@
> > +
> > +#include "MediaConduitInterface.h"
> > +#include "AudioConduit.h"
> > +#include "VideoConduit.h"
> > +#include "../../../trunk/webrtc/modules/video_coding/codecs/interface/video_codec_interface.h"
> 
> Same remark as the cpp file.
Done.

Comment 11

[qiang lu]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> Comment on attachment 8373244 [details] [diff] [review]
> B969395_webrtc_hw_codec.v01.patch
> 
> Review of attachment 8373244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will be a cool feature! :) For your next round of reviews, you should
> ask :doublec (Chris Double) for review because he wrote most of our Android
> OMX code.
> 
> General comments:
> • Check indentation.
> • When long function signatures are wrapped, the new line should be indented
> so the parameter types line up.
> • Trailing whitespace should be removed in Mozilla files. Third-party files
> (like the stagefright headers) should be left as-is.
> • All overridden virtual functions should have an MOZ_OVERRIDE annotation:
> virtual void MyFunction() MOZ_OVERRIDE;
> • You should try to use camelCased names like `videoCodec` in Mozilla code,
> unless you are interfacing with third-party code that uses "snake_case"
> names.
> • Code that is commented out should probably be removed or have a FIXME
> comment with a Bugzilla bug number for follow-up work.
> • Some functions are hardcoded to return WEBRTC_VIDEO_CODEC_OK even when
> calling OMX functions that return err != OK.
> • Some hot functions like Encode() and Decode() are called for every video
> frame, but they call PR_IntervalNow() multiple times just for debug log
> messages. PR_IntervalNow() might be unnecessarily expensive on some
> platforms. Can we #ifdef the PR_IntervalNow() code only for debug builds or
> if some logging macro is #defined?
> 
Done. all above.

> ::: media/webrtc/signaling/signaling.gyp
> @@ +228,5 @@
> > +
> > +		    'cflags': [
> > +              '-Wno-multichar',
> > +          ],
> > +          
> 
> moz.build files are a restricted subset of Python, so indentation can be
> important.
Done.

> 
> ::: media/webrtc/signaling/src/media-conduit/ExtVideoCodec.h
> @@ +6,5 @@
> > +#define ExtVideoCodec_h__
> > +
> > +#include "MediaConduitInterface.h"
> > +
> > +#define EXT_VIDEO_PAYLOAD_TYPE 100
> 
> Why is EXT_VIDEO_PAYLOAD_TYPE needed in a public header? This macro does not
> seem to be used anywhere.
Done. remove it.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +5,5 @@
> > +#include "CSFLog.h"
> > +#include "nspr.h"
> > +
> > +#include <iostream>
> > +//#include "omx_data_dump.h"
> 
> Remove commented code.

Done.

> 
> @@ +12,5 @@
> > +#include "VideoConduit.h"
> > +#include "AudioConduit.h"
> > +#include "../../../trunk/webrtc/video_engine/include/vie_external_codec.h"
> > +#include "runnable_utils.h"
> > +#include "WebrtcExtVideoCodec.h"
> 
> WebrtcExtVideoCodec.h should be the first header #included in
> WebrtcExtVideoCodec.cpp. This ensures WebrtcExtVideoCodec.h is
> self-sufficient, including all the headers and definitions it needs.
Done.

> 
> @@ +25,5 @@
> > +#include <IOMX.h>
> > +#include <OMX_VideoExt.h>
> > +#include <stagefright/MediaCodec.h>
> > +#include <stagefright/MediaDefs.h>
> > +#include <stagefright/MediaErrors.h>
> 
> These #includes should be alphabetized and grouped like:
> WebrtcExtVideoCodec.h, then a blank line, "mozilla/whatever.h" headers, a
> blank line, then system and non-Mozilla headers, a blank line, then
> miscellaneous Mozilla or webrtc headers.
> 
> https://hg.mozilla.org/mozilla-central/file/5f1d4098333f/mfbt/STYLE#l218

Done.

> 
> @@ +35,5 @@
> > +#define EXT_VIDEO_FRAME_HEIGHT 480
> > +#define EXT_VIDEO_MAX_FRAMERATE 30
> > +#define DEQUEUE_BUFFER_TIMEOUT_US (100 * 1000ll) // 100ms
> > +#define START_DEQUEUE_BUFFER_TIMEOUT_US (10 * DEQUEUE_BUFFER_TIMEOUT_US) // 1s
> > +#define MEDIACODEC_VIDEO_MIME_VP8 "video/x-vnd.on2.vp8"
> 
> All these #defines should probably use real static const types.

Done.

> 
> @@ +38,5 @@
> > +#define START_DEQUEUE_BUFFER_TIMEOUT_US (10 * DEQUEUE_BUFFER_TIMEOUT_US) // 1s
> > +#define MEDIACODEC_VIDEO_MIME_VP8 "video/x-vnd.on2.vp8"
> > +
> > +#include <cstdio>
> > +#include <utils/Log.h>
> 
> These #includes should be grouped with the others above, before any #defines.

Done.

> 
> @@ +54,5 @@
> > + 
> > +    looper_ = new ALooper;
> > +    looper_->start();
> > +    msg_ = new AMessage;
> > +    omx_ = MediaCodec::CreateByType(looper_, mime, encoder);
> 
> Can MediaCodec::CreateByType() fail? Do we need to handle any errors here or
> check for null omx_ in other member functions?

Done, check mOmx is not nullptr in other member functions.

> 
> @@ +59,5 @@
> > +  }
> > +
> > +  virtual ~WebrtcOMX() {
> > +	  msg_.clear();
> > +    looper_.clear();
> 
> Fix indentation.

Done.

> 
> @@ +64,5 @@
> > +    omx_.clear();
> > +  }
> > +
> > +  status_t Configure(const sp<Surface>& nativeWindow,
> > +    const sp<ICrypto>& crypto, uint32_t flags) {
> 
> When long function signatures are wrapped, the new line should be indented
> so the parameter types line up.

Done.

> 
> @@ +78,5 @@
> > +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +
> > +    status_t err = omx_->start();
> > +	  CSFLogDebug(logTag, "WebrtcOMX::%s, omx_->start() return err = %d",
> > +               __FUNCTION__, err);
> 
> Fix indentation.
> 

Done.

> @@ +99,5 @@
> > +
> > +  friend class WebrtcExtVideoEncoder;
> > +  friend class WebrtcExtVideoDecoder;
> > +
> > +  sp<ALooper> looper_;
> 
> Make these member variables private.
> 

Done.

> @@ +118,5 @@
> > +    callback_(nullptr),
> > +    encoder_(nullptr) {
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +//  dataEncoderDump_ = new OMXDataDump();
> > +//  dataEncoderDump_->Init("video/x-vnd.on2.vp8");
> 
> Remove commented code.

Done.

> 
> @@ +135,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);
> 
> Why is all this code commented out? It should be removed or have a FIXME
> comment with a Bugzilla bug number for follow-up work.

I just enabled basic vp8 hardware configurations in this patch since some of those configurations are not support well in our device driver when i implemented this patch.

And once i confirm that those commented configurations are implemented correctly, i will make them back.

> 
> @@ +158,5 @@
> > +    uint32_t maxPayloadSize) {
> > +  max_payload_size_ = maxPayloadSize;
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +
> > +  if (omxEnc == nullptr) { 
> 
> Just check `if (!omxEnc)`, not `== nullptr`.

Done.

> 
> @@ +160,5 @@
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +
> > +  if (omxEnc == nullptr) { 
> > +    // FIXME: implement proper lifecycle management
> > +	  omxEnc = new WebrtcOMX("video/x-vnd.on2.vp8", true /* encoder */);
> 
> Replace hardcoded string with MEDIACODEC_VIDEO_MIME_VP8 (from above)?

Done.

> 
> @@ +164,5 @@
> > +	  omxEnc = new WebrtcOMX("video/x-vnd.on2.vp8", true /* encoder */);
> > +    // FIXME: use input parameters from webrtc negotiation
> > +    webrtc::VideoCodec codec_inst;
> > +    memset(&codec_inst, 0, sizeof(webrtc::VideoCodec));
> > +    strncpy(codec_inst.plName, EXT_VIDEO_PLAYLOAD_NAME, 31);
> 
> Where does the magic number 31 come from? Should you use
> MOZ_ARRAY_LENGTY(EXT_VIDEO_PLAYLOAD_NAME)? EXT_VIDEO_PLAYLOAD_NAME is "VP8",
> so strncpy() is never going to copy 31 chars.

Done.

> 
> @@ +177,5 @@
> > +
> > +    VideoCodecSettings2AMessage(omxEnc->msg_, &codec_inst, MEDIACODEC_VIDEO_MIME_VP8, true /*encoder*/);
> > +    
> > +    omxEnc->Configure(nullptr, nullptr,
> > +      MediaCodec::CONFIGURE_FLAG_ENCODE);
> 
> Can Configure() fail? Also, remove trailing whitespace and I don't think you
> need to line wrap here.

Done.

> 
> @@ +186,5 @@
> > +  // TODO: eliminate extra pixel copy & color conversion
> > +  size_t size = EXT_VIDEO_FRAME_WIDTH * EXT_VIDEO_FRAME_HEIGHT * 3 / 2;
> > +  if (encoded_image_._size < size) {
> > +    if (encoded_image_._buffer) {
> > +      delete [] encoded_image_._buffer;
> 
> You can remove this null _buffer check because C++ allows you to safely call
> delete or delete[] on a null pointer.

Done.

> 
> @@ +383,5 @@
> > +  return WEBRTC_VIDEO_CODEC_OK;
> > +}
> > +
> > +// TODO: eliminate global variable after implementing prpper lifecycle management code
> > +static WebrtcOMX* omxDec = nullptr;
> 
> Why can't omxDec be a member variable of WebrtcExtVideoDecoder?

I will try to refactory the lifecycle management of those global variable(omxEnc, omxDec) and send another patch to review.

> 
> @@ +397,5 @@
> > +    int32_t numberOfCores) {
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +  // FIXME: implement proper lifecycle management
> > +  if (omxDec == nullptr) {
> > +	omxDec  = new WebrtcOMX("video/x-vnd.on2.vp8", false /* decoder */);
> 
> Replace hardcoded string with MEDIACODEC_VIDEO_MIME_VP8 (from above)?

Done.

> 
> @@ +401,5 @@
> > +	omxDec  = new WebrtcOMX("video/x-vnd.on2.vp8", false /* decoder */);
> > +    // FIXME: use input parameters from webrtc negotiation
> > +    webrtc::VideoCodec codec_inst;
> > +    memset(&codec_inst, 0, sizeof(webrtc::VideoCodec));
> > +    strncpy(codec_inst.plName, EXT_VIDEO_PLAYLOAD_NAME, 31);
> 
> Where does the magic number 31 come from? Should you use
> MOZ_ARRAY_LENGTY(EXT_VIDEO_PLAYLOAD_NAME)? EXT_VIDEO_PLAYLOAD_NAME is "VP8",
> so strncpy() is never going to copy 31 chars.

Done.

> 
> @@ +419,5 @@
> > +               __FUNCTION__, err);
> > +    if (err != OK) {
> > +		CSFLogDebug(logTag,  "MediaCodecDecoderImpl::%s, decoder configure return err = %d",
> > +               __FUNCTION__, err);
> > +	}
> 
> Fix indentation.

Done.

> 
> @@ +424,5 @@
> > +  }
> > +  omxDec->started_ = false;
> > +  decoder_ = omxDec;
> > +
> > +  return WEBRTC_VIDEO_CODEC_OK;
> 
> Why do we return WEBRTC_VIDEO_CODEC_OK even when omxDec->Configure() returns
> err != OK?

Done. return WEBRTC_VIDEO_CODEC_ERROR immediately when omxDec->Configure() returns not OK.

> 
> @@ +428,5 @@
> > +  return WEBRTC_VIDEO_CODEC_OK;
> > +}
> > +
> > +void generateVideoFrame_sw(EncodedFrame* frame, const sp<ABuffer>& decoded,
> > +  webrtc::I420VideoFrame* videoFrame) {
> 
> Make function static and capitalized. Fix wrapped line indentation and
> trailing whitespace.

Done.

> 
> @@ +469,5 @@
> > +
> > +  videoFrame->set_timestamp(frame->timestamp_);
> > +}
> > +
> > +status_t feedOMXInput(WebrtcOMX* decoder, const sp<MediaCodec>& omx,
> 
> Make function static and capitalized.

Done.

> 
> @@ +504,5 @@
> > +
> > +  return err;
> > +}
> > +
> > +status_t getOMXOutput(WebrtcOMX* decoder, const sp<MediaCodec>& omx,
> 
> Make function static and capitalized.

Done.

> 
> @@ +577,5 @@
> > +  }
> > +
> > +  uint32_t time = PR_IntervalNow();
> > +  WebrtcOMX* decoder = static_cast<WebrtcOMX*>(decoder_);
> > +  if (!decoder->started_) {
> 
> Move `time = PR_IntervalNow()` instead the `if (!decoder->started_)` block.
> We don't want to call PR_IntervalNow() for *every* video frame when we only
> use it to log one debug message when calling start().

Done.

> 
> @@ +595,5 @@
> > +    } while (err == INFO_OUTPUT_BUFFERS_CHANGED);
> > +  }
> > +
> > +  CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() end", __FUNCTION__);
> > +  return WEBRTC_VIDEO_CODEC_OK;
> 
> Why do we return WEBRTC_VIDEO_CODEC_OK when feedOMXInput() or getOMXOutput()
> might return errors?

Done. return WEBRTC_VIDEO_CODEC_ERROR if feedOMXInput() or getOMXOutput() return errors.

> 
> ::: media/webrtc/signaling/test/Makefile.in
> @@ +159,5 @@
> >  ifeq ($(OS_TARGET), Android)
> >  LOCAL_INCLUDES +=  -I$(topsrcdir)/media/mtransport/third_party/nrappkit/src/port/android/include
> > +LIBS += \
> > +	-L$(DEPTH)/media/omx-plugin/lib/kk/libutils \
> > +  -lutils \
> 
> Fix -L indentation.

Done.

> 
> ::: toolkit/library/Makefile.in
> @@ +125,5 @@
> >  
> >  ifdef MOZ_WEBRTC
> > +OS_LIBS += \
> > +	-L$(DEPTH)/media/omx-plugin/lib/kk/libutils \
> > +  -lutils \
> 
> Fix -L indentation.

Done.

> 
> ::: toolkit/toolkit.mozbuild
> @@ +79,5 @@
> >  if CONFIG['MOZ_OMX_PLUGIN']:
> >      add_tier_dir('platform', [
> > +        'media/omx-plugin/lib/kk/libutils',
> > +        'media/omx-plugin/lib/kk/libstagefright_foundation',
> > +        'media/omx-plugin/lib/kk/libstagefright',
> 
> libstagefright_foundation should be listed after libstagefright.

Don't fix. 
If i move line libstagefright_foundation after libstagefright, libstagefright couldn't build correctly since it depends on libstagefright_foundation.

Comment 12

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v02.patch

Still have some TODO should be fixed in this patch.
    1. Fix bug 973761
    2. get correctly resolution and video codec type from VideoConduit.cpp
    3. enabled full supported vp8 hardware configurations in VideoCodecSettings2AMessage() fucntion
    4. eliminate global variable(omxEnc & omxDec) after implementing prpper lifecycle management code
    5. try to eliminate extra pixel copy & color conversion exist in encode & decode function
    6. try to implement SetChannelParameters and SetRates
    7. make sure it can work more stable on Intel(merrifield PR2) & QCOM (Nexus 5) devices

Comment 13

[Andreas Gal :gal]

Whats the followup work here to enable this on FFOS as well and to enable H264?

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to qiang lu from comment #12)
> Created attachment 8378044 [details] [diff] [review]
> B969395_webrtc_hw_codec.v02.patch
> 
> Still have some TODO should be fixed in this patch.
>     1. Fix bug 973761
>     2. get correctly resolution and video codec type from VideoConduit.cpp
>     3. enabled full supported vp8 hardware configurations in
> VideoCodecSettings2AMessage() fucntion
>     4. eliminate global variable(omxEnc & omxDec) after implementing prpper
> lifecycle management code
>     5. try to eliminate extra pixel copy & color conversion exist in encode
> & decode function
>     6. try to implement SetChannelParameters and SetRates
>     7. make sure it can work more stable on Intel(merrifield PR2) & QCOM
> (Nexus 5) devices

Qiang, do you think any of those tasks can be postponed to follow-up bugs? This is a big patch and I don't want to make you wait any longer than necessary to land the basic functionality. Tasks #3–6 look like they be good candidates for fixing in a follow-up bug.

Regarding Andreas' comment 13: do you know of plans to add Firefox OS support?

Comment 15

[Chris Peterson [:cpeterson]]

Comment on attachment 8378044 [details] [diff] [review]
B969395_webrtc_hw_codec.v02.patch

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

Qiang: your patch v2 looks very good! I am giving your patch feedback+ instead of review+ because I think :doublec is the person who should give the final review.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +536,5 @@
> +  // FIXME : see bug 973761, we need implement a mechanism that enable/disable hw codec on demand.
> +  CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> +              payloadName.c_str(), video_codec.plType);
> +  VideoEncoder* extEncoder = ExtVideoCodec::CreateEncoder();
> +  SetExternalSendCodec(120, extEncoder);

What is the magic number 120? The VP8 payload type? Please add a #define or const somewhere (even if only in this .cpp file) to give this magic number a friendly name.

@@ +686,5 @@
>      }//end for codeclist
>  
>      CSFLogDebug(logTag, "%s, Before ExtVideoCodec codec %s(%d)", __FUNCTION__,
>                          video_codec.plName, video_codec.plType);
> +                        

Please remove trailing whitespace.

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +50,5 @@
>   
> +    mLooper = new ALooper;
> +    mLooper->start();
> +    mMsg = new AMessage;
> +    mOmx = MediaCodec::CreateByType(mLooper, mime, encoder);

Can mOmx initialization can fail? If so, can you move this initialization to WebrtcOMX's Configure() or Start() functions? You can return a status_t error code from those functions. Then you could remove all other mOmx null checks because you could assume Configure() or Start() succeeded (otherwise the call should not be using this WebrtcOMX object).

@@ +76,5 @@
>  
>    status_t Start() {
>      CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +    status_t err = BAD_VALUE;
> +    

Please remove trailing whitespace.

@@ +102,5 @@
>  
> +  Vector<sp<ABuffer>>* getInput() {
> +    return &mInput;
> +  }
> +  

Please remove trailing whitespace.

@@ +138,5 @@
> +  size_t sizeY = inputImage.allocated_size(webrtc::kYPlane);
> +  size_t sizeUV = inputImage.allocated_size(webrtc::kUPlane);
> +  const uint8_t* u = inputImage.buffer(webrtc::kUPlane);
> +  const uint8_t* v = inputImage.buffer(webrtc::kVPlane);
> +  

Please remove trailing whitespace.

@@ +250,5 @@
>    if (err != OK) {
>      CSFLogError(logTag,  "%s WebrtcExtVideoEncoder::Encode() dequeue OMX input buffer error:%d", __FUNCTION__, err);
>      return WEBRTC_VIDEO_CODEC_ERROR;
>    }
> +  

Please remove trailing whitespace.

@@ +267,5 @@
>    for (int i = 0; i < sizeUV;
>        i++, dstUV++, u++, v++) {
>      *dstUV = (*v << 8) + *u;
>    }
> +  */

Can you remove this commented out code now that you are calling the I420toNV12() function?

@@ +281,5 @@
>      CSFLogError(logTag,  "%s WebrtcExtVideoEncoder::Encode() queue OMX input buffer error:%d", __FUNCTION__, err);
>      return WEBRTC_VIDEO_CODEC_ERROR;
>    }
> +
> +#ifdef DEBUG  

Please remove trailing whitespace.

@@ +338,4 @@
>      webrtc::kKeyFrame:webrtc::kDeltaFrame;
>  
> +  CSFLogDebug(logTag,  "%s WebrtcExtVideoEncoder::Encode() frame type:%d size:%u", __FUNCTION__, mEncodedImage._frameType, mEncodedImage._length);
> +  

Please remove trailing whitespace.

@@ +368,5 @@
>    return WEBRTC_VIDEO_CODEC_OK;
>  }
>  
>  int32_t WebrtcExtVideoEncoder::Release() {
> +  

Please remove trailing whitespace.

@@ +378,5 @@
>      //encoder_ = nullptr;
>    }
>  
> +  if (mEncodedImage._buffer) {
> +    delete [] mEncodedImage._buffer;

You can remove this mEncodedImage._buffer null check because it is safe to call `delete [] null`.

@@ +518,3 @@
>    static int64_t firstTime = -1ll;
>    size_t index;
> +  

Please remove trailing whitespace.

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

Please remove trailing whitespace.

@@ +662,5 @@
>    }
>  
> +  CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() end, err = %d", __FUNCTION__, err);
> +  
> +  if (err != OK) 

Mozilla coding style is that `if` statements should always have curly braces {}.

@@ +664,5 @@
> +  CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() end, err = %d", __FUNCTION__, err);
> +  
> +  if (err != OK) 
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  

Please remove trailing whitespace.

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h
@@ +75,5 @@
>  
>    // Implement VideoDecoder interface.
>    virtual int32_t InitDecode(const webrtc::VideoCodec* codecSettings,
> +                              int32_t numberOfCores) MOZ_OVERRIDE;
> +                              

Please remove trailing whitespace.

@@ +82,5 @@
> +                          const webrtc::RTPFragmentationHeader* fragmentation,
> +                          const webrtc::CodecSpecificInfo*
> +                          codecSpecificInfo = NULL,
> +                          int64_t renderTimeMs = -1) MOZ_OVERRIDE;
> +                          

Please remove trailing whitespace.

Comment 16

[qiang lu]

(In reply to Andreas Gal :gal from comment #13)
> Whats the followup work here to enable this on FFOS as well and to enable
> H264?

AFAIK, John Lin is focusing on it.
see, 
bug 879668
and
bug 911046

Comment 17

[qiang lu]

(In reply to Chris Peterson (:cpeterson) from comment #14)
> (In reply to qiang lu from comment #12)
> > Created attachment 8378044 [details] [diff] [review]
> > B969395_webrtc_hw_codec.v02.patch
> > 
> > Still have some TODO should be fixed in this patch.
> >     1. Fix bug 973761
> >     2. get correctly resolution and video codec type from VideoConduit.cpp
> >     3. enabled full supported vp8 hardware configurations in
> > VideoCodecSettings2AMessage() fucntion
> >     4. eliminate global variable(omxEnc & omxDec) after implementing prpper
> > lifecycle management code
> >     5. try to eliminate extra pixel copy & color conversion exist in encode
> > & decode function
> >     6. try to implement SetChannelParameters and SetRates
> >     7. make sure it can work more stable on Intel(merrifield PR2) & QCOM
> > (Nexus 5) devices
> 
> Qiang, do you think any of those tasks can be postponed to follow-up bugs?
> This is a big patch and I don't want to make you wait any longer than
> necessary to land the basic functionality. Tasks #3–6 look like they be good
> candidates for fixing in a follow-up bug.
Got it.
I am working on tasks #1-2, and will fire some bugs to track tasks #3-6.

> 
> Regarding Andreas' comment 13: do you know of plans to add Firefox OS
> support?
AFAIK, John Lin is focusing on it.
see, bug 879668 and bug 911046.

And, I am very interesting in enabling the webrtc hardware codec support on Firefox OS.

Comment 18

[Chris Peterson [:cpeterson]]

Comment on attachment 8378044 [details] [diff] [review]
B969395_webrtc_hw_codec.v02.patch

oops: I accidentally removed doublec's r? instead of mine.

Comment 19

[Andreas Gal :gal]

Bug 911046 overlaps a lot with this bug so I am sure we can use this work here as an easy basis to enable FFOS support as well. Qiang, would it help if we make FFOS hardware available to you for testing?

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8378044 [details] [diff] [review]
B969395_webrtc_hw_codec.v02.patch

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

Starting to look good! But I think this patch still hardcodes the captured video as being 640x480@30fps, which isn't guaranteed at all? That should be fixed. 

You probably also want to see if your editor or whatever you use to generate diffs can show whitespace errors :)

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +32,5 @@
>  
> +static const char EXT_VIDEO_PLAYLOAD_NAME[] = "VP8";
> +static const int EXT_VIDEO_FRAME_WIDTH = 640;
> +static const int EXT_VIDEO_FRAME_HEIGHT = 480;
> +static const int EXT_VIDEO_MAX_FRAMERATE = 30;

Need to get rid of this and get them from the upper layer (as you said previously).

@@ +55,4 @@
>    }
>  
>    virtual ~WebrtcOMX() {
> +    if (mMsg != nullptr) 

When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr. 
(https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)

Also trailing whitespace at the end.

@@ +68,3 @@
>      CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +    status_t err = BAD_VALUE;
> +    if (mOmx != nullptr) {

same

@@ +77,5 @@
>    status_t Start() {
>      CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +    status_t err = BAD_VALUE;
> +    
> +    if (mOmx != nullptr) {

same

@@ +155,4 @@
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
>    format->setString("mime", mime);
>  //  format->setInt32("store-metadata-in-buffers", 0);

What's the idea behind the commented out stuff?

@@ +185,3 @@
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>  
> +  if (!omxEnc) { 

trailing whitespace

@@ +204,1 @@
>      

trailing whitespace

@@ +503,5 @@
>          *dstV++ = *uv++;
>        }
>        uv += padding;
>      }
>    }

Make an I420toNV12?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8378044 [details] [diff] [review]
B969395_webrtc_hw_codec.v02.patch

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

Partial review/comments:

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -537,5 @@
> -  if (video_codec.plType == 120) {
> -    CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> -                        payloadName.c_str(), video_codec.plType);
> -    VideoEncoder* extEncoder = ExtVideoCodec::CreateEncoder();
> -    SetExternalSendCodec(120, extEncoder);

I should note that this patch obviously assumes an earlier patch which has problems - current trunk doesn't have any magic numbers like 120.  I assume this is a test for "is this VP8?", we need a more definitive way to determine that - payload values are (basically) arbitrary and not hard-assigned to codecs.

@@ +691,5 @@
> +    // FIXME : see bug 973761, we need implement a mechanism that enable/disable hw codec on demand.
> +    CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> +                video_codec.plName, video_codec.plType);
> +    VideoDecoder* extDecoder = ExtVideoCodec::CreateDecoder();
> +    SetExternalRecvCodec(120, extDecoder);

Same issue.

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ -7,3 @@
>  
> -#include <iostream>
> -//#include "omx_data_dump.h"

This modifies a file not in the tree: where's the patch with the base file?

@@ +58,5 @@
> +    if (mMsg != nullptr) 
> +      mMsg.clear();
> +    if (mLooper != nullptr)
> +      mLooper.clear();
> +    if (mOmx != nullptr)

Normal mozilla style is "always brace an if"

@@ +85,2 @@
>  
> +      isStarted = err == OK;

Please don't use identifiers likely to clash with other includes like "OK"

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

Shouldn't this be the starting bitrate, not the min bitrate?

@@ +161,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);

Why are these commented out?

@@ +169,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);
>    if (isEncoder) {
> +    format->setInt32("i-frame-interval", 100); // one I-frame per sec

One iframe per second?  In a video call, recovery should be handled by normal mechanisms (PLI/NACK/etc), not periodic iframes

@@ +188,1 @@
>      // FIXME: implement proper lifecycle management

I assume this will need to be fixed before checkin

@@ +190,1 @@
>      // FIXME: use input parameters from webrtc negotiation

Please file a bug indicating what parameters you need from the signaling code so we can add them to the codec interface/etc as needed

@@ +215,5 @@
>    size_t size = EXT_VIDEO_FRAME_WIDTH * EXT_VIDEO_FRAME_HEIGHT * 3 / 2;
> +  if (mEncodedImage._size < size) {
> +    delete [] mEncodedImage._buffer;
> +    mEncodedImage._buffer = new uint8_t[size];
> +    mEncodedImage._size = size;

Is this supposed to be a compressed buffer?  The name suggests it.  If so, this calculation is usually WAY higher than needed, and can be smaller than needed in some odd cases.  If it's a copy for the source data, there are better ways to calculate the size (including stride/rounding concerns if any; depending on how it's used.  

Also please add some comments, and comment that this is 4:2:0 encoding (which I'm assuming it is).

@@ +237,1 @@
>    WebrtcOMX* encoder = (reinterpret_cast<WebrtcOMX*>(encoder_));

Is there a reason we need to reinterpret here?

@@ +275,5 @@
> +#ifdef DEBUG
> +  time = PR_IntervalNow();
> +#endif
> +
> +  err = omx->queueInputBuffer(index, 0, size, inputImage.render_time_ms() * 1000000 /* ms to us */, 0);

isn't ms * 1000000 nanoseconds (ns), not us?  Perhaps you mean * 1000?

@@ +374,5 @@
>    if (encoder_) {
>      // FIXME: Leak! Should implement proper lifecycle management
>      // WebrtcOMX* omx = static_cast<WebrtcOMX*>(encoder_);
>      // delete omx;
>      //encoder_ = nullptr;

Again, I assume this will be dealt with

@@ +378,5 @@
>      //encoder_ = nullptr;
>    }
>  
> +  if (mEncodedImage._buffer) {
> +    delete [] mEncodedImage._buffer;

cpeterson: "You can remove this mEncodedImage._buffer null check because it is safe to call `delete [] null`."

You *can* remove it; possibly it would be more efficient to use it on some architectures/compilers since there are other writes and a function call that will be suppressed if we do the check.  Not likely this is very important though, and so simplicity might be better.

@@ +437,1 @@
>      codec_inst.plType = 120;

hard-coded 120 again

@@ +474,4 @@
>    size_t width = frame->width_;
>    size_t height = frame->height_;
>    size_t widthUV = width / 2;
> +  if (videoFrame->CreateEmptyFrame(width, height, width, widthUV, widthUV)) {

Are there any restrictions on width/height, or stride issues with these hardware formats?

@@ +484,2 @@
>    size_t roundedSliceHeight = (height + 31) & ~31;
> +  //  size_t roundedSliceHeight = height;

Why do we need to round to multiple of 32 here?  Please comment as to why

@@ +503,5 @@
>          *dstV++ = *uv++;
>        }
>        uv += padding;
>      }
>    }

gcp: "Make an I420toNV12?"

Or check if there's one in lubyuv

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h
@@ +65,2 @@
>  
>    void* encoder_;

void *????

@@ +100,2 @@
>  
> +  void* mDecoder;

void *????

If these are opaque, please define an opaque pointer type, not void *.

Comment 22

[cajbir (:cajbir)]

Comment on attachment 8378044 [details] [diff] [review]
B969395_webrtc_hw_codec.v02.patch

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

Looks good, just need clarification on the 'kk' libstagefright things in Makefile.in.

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +116,5 @@
> +  sp<MediaCodec> mOmx; // MediaCodec
> +  sp<AMessage> mMsg;
> +  bool isStarted;
> +  Vector<sp<ABuffer>> mInput;
> +  Vector<sp<ABuffer>> mOutput;

Please add comments explaining what these members are for. You can remove the "// MediaCodec" since that provides nothing over what the actual type information conveys.

::: toolkit/library/Makefile.in
@@ +130,5 @@
>    -L$(DEPTH)/media/omx-plugin/lib/kk/libstagefright_foundation \
>    -lstagefright_foundation \
>    -L$(DEPTH)/media/omx-plugin/lib/kk/libstagefright \
>    -lstagefright \
>    $(NULL)

Is this based on a prior patch? The Makefile.in in my tree doesn't include any of the "lib/kk/libstagefright" files that this patch changes. Same with the other Makefile.in change.

This uses 'kk' directly whereas other parts of the code use "-lstagefright". Won't these conflict? Is WEBRTC going to be kitkat only?

Comment 23

[qiang lu]

Attachment: updated according to jesup comment21

Comment 24

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v03.patch

Comment 25

[qiang lu]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #20)
> Comment on attachment 8378044 [details] [diff] [review]
> B969395_webrtc_hw_codec.v02.patch
> 
> Review of attachment 8378044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Starting to look good! But I think this patch still hardcodes the captured
> video as being 640x480@30fps, which isn't guaranteed at all? That should be
> fixed. 
> 
Done. get real resolution and fps settings from up layer(VideoConduit.cpp). 

> You probably also want to see if your editor or whatever you use to generate
> diffs can show whitespace errors :)
Done. I reset my vim to highlight those whitespace errors.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +32,5 @@
> >  
> > +static const char EXT_VIDEO_PLAYLOAD_NAME[] = "VP8";
> > +static const int EXT_VIDEO_FRAME_WIDTH = 640;
> > +static const int EXT_VIDEO_FRAME_HEIGHT = 480;
> > +static const int EXT_VIDEO_MAX_FRAMERATE = 30;
> 
> Need to get rid of this and get them from the upper layer (as you said
> previously).
> 
Done. remove them.
> @@ +55,4 @@
> >    }
> >  
> >    virtual ~WebrtcOMX() {
> > +    if (mMsg != nullptr) 
> 
> When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr
> or myPtr == nullptr. 
> (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)
> 
> Also trailing whitespace at the end.
Done.
> 
> @@ +68,3 @@
> >      CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +    status_t err = BAD_VALUE;
> > +    if (mOmx != nullptr) {
> 
> same

Done.

> 
> @@ +77,5 @@
> >    status_t Start() {
> >      CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +    status_t err = BAD_VALUE;
> > +    
> > +    if (mOmx != nullptr) {
> 
> same

Done.

> 
> @@ +155,4 @@
> >    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> >  
> >    format->setString("mime", mime);
> >  //  format->setInt32("store-metadata-in-buffers", 0);
> 
> What's the idea behind the commented out stuff?

I don't make sure whether those commented settings is ready for using when i test this patch on our device. So leave them there for the moment. I will fire a bug to track those encode/decode setting support issue.

> 
> @@ +185,3 @@
> >    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> >  
> > +  if (!omxEnc) { 
> 
> trailing whitespace

Done.

> 
> @@ +204,1 @@
> >      
> 
> trailing whitespace
> 
> @@ +503,5 @@
> >          *dstV++ = *uv++;
> >        }
> >        uv += padding;
> >      }
> >    }
> 
> Make an I420toNV12?

Done. using libyuv::I420ToNV12 instead.

Comment 26

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #21)
> Comment on attachment 8378044 [details] [diff] [review]
> B969395_webrtc_hw_codec.v02.patch
> 
> Review of attachment 8378044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Partial review/comments:
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ -537,5 @@
> > -  if (video_codec.plType == 120) {
> > -    CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> > -                        payloadName.c_str(), video_codec.plType);
> > -    VideoEncoder* extEncoder = ExtVideoCodec::CreateEncoder();
> > -    SetExternalSendCodec(120, extEncoder);
> 
> I should note that this patch obviously assumes an earlier patch which has
> problems - current trunk doesn't have any magic numbers like 120.  I assume
> this is a test for "is this VP8?", we need a more definitive way to
> determine that - payload values are (basically) arbitrary and not
> hard-assigned to codecs.

Done. remove this VP8 codec type check. HW enable check should be done on B973761.

> 
> @@ +691,5 @@
> > +    // FIXME : see bug 973761, we need implement a mechanism that enable/disable hw codec on demand.
> > +    CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> > +                video_codec.plName, video_codec.plType);
> > +    VideoDecoder* extDecoder = ExtVideoCodec::CreateDecoder();
> > +    SetExternalRecvCodec(120, extDecoder);
> 
> Same issue.

Ditto

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ -7,3 @@
> >  
> > -#include <iostream>
> > -//#include "omx_data_dump.h"
> 
> This modifies a file not in the tree: where's the patch with the base file?
> 

Done. remove it. it's only used for local debugging.

> @@ +58,5 @@
> > +    if (mMsg != nullptr) 
> > +      mMsg.clear();
> > +    if (mLooper != nullptr)
> > +      mLooper.clear();
> > +    if (mOmx != nullptr)
> 
> Normal mozilla style is "always brace an if"
> 

Done.

> @@ +85,2 @@
> >  
> > +      isStarted = err == OK;
> 
> Please don't use identifiers likely to clash with other includes like "OK"
> 
Done.

> @@ +156,5 @@
> >  
> >    format->setString("mime", mime);
> >  //  format->setInt32("store-metadata-in-buffers", 0);
> >  //  format->setInt32("prepend-sps-pps-to-idr-frames", 0);
> >    format->setInt32("bitrate", codecSettings->minBitrate * 1000); // kbps->bps
> 
> Shouldn't this be the starting bitrate, not the min bitrate?

Done. change it to startBitrate.

> @@ +161,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);
> 
> Why are these commented out?

I don't make sure whether those commented settings is ready for using when i test this patch on our device. So leave them there for the moment. I will fire a bug to track those encode/decode setting support status for different codec vendor.

> 
> @@ +169,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);
> >    if (isEncoder) {
> > +    format->setInt32("i-frame-interval", 100); // one I-frame per sec
> 
> One iframe per second?  In a video call, recovery should be handled by
> normal mechanisms (PLI/NACK/etc), not periodic iframes
Done. change it to KINT32MAX instead. it means that i-frame should be generated on demand.

> 
> @@ +188,1 @@
> >      // FIXME: implement proper lifecycle management
> 
> I assume this will need to be fixed before checkin

Done. refactor code. make encode/decode processing is managed by WebrtcExtVideoEncoder/WebrtcExtVideoDecoder instance.

> 
> @@ +190,1 @@
> >      // FIXME: use input parameters from webrtc negotiation
> 
> Please file a bug indicating what parameters you need from the signaling
> code so we can add them to the codec interface/etc as needed

Done. don't fire bug, just get width/height etc. information from input parameters.

> 
> @@ +215,5 @@
> >    size_t size = EXT_VIDEO_FRAME_WIDTH * EXT_VIDEO_FRAME_HEIGHT * 3 / 2;
> > +  if (mEncodedImage._size < size) {
> > +    delete [] mEncodedImage._buffer;
> > +    mEncodedImage._buffer = new uint8_t[size];
> > +    mEncodedImage._size = size;
> 
> Is this supposed to be a compressed buffer?  The name suggests it.  If so,
> this calculation is usually WAY higher than needed, and can be smaller than
> needed in some odd cases.  If it's a copy for the source data, there are
> better ways to calculate the size (including stride/rounding concerns if
> any; depending on how it's used.  
> 
> Also please add some comments, and comment that this is 4:2:0 encoding
> (which I'm assuming it is).

Done. this buffer is used for storing encoded frame which should be 4:2:0. i use  webrtc::CalcBufferSize to calculate needed buffer size.

> 
> @@ +237,1 @@
> >    WebrtcOMX* encoder = (reinterpret_cast<WebrtcOMX*>(encoder_));
> 
> Is there a reason we need to reinterpret here?

Done. remove it.

> 
> @@ +275,5 @@
> > +#ifdef DEBUG
> > +  time = PR_IntervalNow();
> > +#endif
> > +
> > +  err = omx->queueInputBuffer(index, 0, size, inputImage.render_time_ms() * 1000000 /* ms to us */, 0);
> 
> isn't ms * 1000000 nanoseconds (ns), not us?  Perhaps you mean * 1000?
> 
Done. change it from 100000 to 1000.

> @@ +374,5 @@
> >    if (encoder_) {
> >      // FIXME: Leak! Should implement proper lifecycle management
> >      // WebrtcOMX* omx = static_cast<WebrtcOMX*>(encoder_);
> >      // delete omx;
> >      //encoder_ = nullptr;
> 
> Again, I assume this will be dealt with

Done. remove this potential leak code by refactor encode/decode instance management. 

> 
> @@ +378,5 @@
> >      //encoder_ = nullptr;
> >    }
> >  
> > +  if (mEncodedImage._buffer) {
> > +    delete [] mEncodedImage._buffer;
> 
> cpeterson: "You can remove this mEncodedImage._buffer null check because it
> is safe to call `delete [] null`."
> 
> You *can* remove it; possibly it would be more efficient to use it on some
> architectures/compilers since there are other writes and a function call
> that will be suppressed if we do the check.  Not likely this is very
> important though, and so simplicity might be better.
> 

Done.

> @@ +437,1 @@
> >      codec_inst.plType = 120;
> 
> hard-coded 120 again
> 
Done. remove it.

> @@ +474,4 @@
> >    size_t width = frame->width_;
> >    size_t height = frame->height_;
> >    size_t widthUV = width / 2;
> > +  if (videoFrame->CreateEmptyFrame(width, height, width, widthUV, widthUV)) {
> 
> Are there any restrictions on width/height, or stride issues with these
> hardware formats?

AFAIK, our hardware doesn't have special restrictions on width/height. And i changed this convert code to use the libyuv:NV12ToI420 function of libyuv.

> 
> @@ +484,2 @@
> >    size_t roundedSliceHeight = (height + 31) & ~31;
> > +  //  size_t roundedSliceHeight = height;
> 
> Why do we need to round to multiple of 32 here?  Please comment as to why

Done. remove those codes.

> 
> @@ +503,5 @@
> >          *dstV++ = *uv++;
> >        }
> >        uv += padding;
> >      }
> >    }
> 
> gcp: "Make an I420toNV12?"
> 
> Or check if there's one in lubyuv

Done. it should be NV12 to I420, i changed code to use the function libyuv::NV12ToI420 of libyuv directly.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h
> @@ +65,2 @@
> >  
> >    void* encoder_;
> 
> void *????

Done. changed to WebrtcOMX point

> 
> @@ +100,2 @@
> >  
> > +  void* mDecoder;
> 
> void *????
> 
> If these are opaque, please define an opaque pointer type, not void *.

Done. changed to WebrtcOMX type point

Comment 27

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v03.patch

fixs issues accroding to comments(20,21) from  gcp and jesup.

Comment 28

[qiang lu]

(In reply to Chris Double (:doublec) from comment #22)
> Comment on attachment 8378044 [details] [diff] [review]
> B969395_webrtc_hw_codec.v02.patch
> 
> Review of attachment 8378044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just need clarification on the 'kk' libstagefright things in
> Makefile.in.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +116,5 @@
> > +  sp<MediaCodec> mOmx; // MediaCodec
> > +  sp<AMessage> mMsg;
> > +  bool isStarted;
> > +  Vector<sp<ABuffer>> mInput;
> > +  Vector<sp<ABuffer>> mOutput;
> 
> Please add comments explaining what these members are for. You can remove
> the "// MediaCodec" since that provides nothing over what the actual type
> information conveys.

I will add comments to those variables.

> 
> ::: toolkit/library/Makefile.in
> @@ +130,5 @@
> >    -L$(DEPTH)/media/omx-plugin/lib/kk/libstagefright_foundation \
> >    -lstagefright_foundation \
> >    -L$(DEPTH)/media/omx-plugin/lib/kk/libstagefright \
> >    -lstagefright \
> >    $(NULL)
> 
> Is this based on a prior patch? The Makefile.in in my tree doesn't include
> any of the "lib/kk/libstagefright" files that this patch changes. Same with
> the other Makefile.in change.

it's my fault in last patch(i miss something in there). i will send another patch for you review.

> 
> This uses 'kk' directly whereas other parts of the code use "-lstagefright".
> Won't these conflict? Is WEBRTC going to be kitkat only?

I add OS type check to toolkit/library/Makefile.in and want to build it with those stub library on Android build only. whether i should add more type check when build libxul ?

And AFAIK webrtc HW acceleration is kitkat only. it could be enabled on those device which running android kitkat with Android SDK >= 19 and HW codec support.

Comment 29

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8386182 [details] [diff] [review]
B969395_webrtc_hw_codec.v03.patch

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

There's a problem with cdoubles bugzilla account, so I'm going to (temporarily) remove him from reviewers or else this bug is effectively not editable :-/

Comment 30

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8386182 [details] [diff] [review]
B969395_webrtc_hw_codec.v03.patch

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

...and Bugzilla lost my initial review comments :(

The code is starting to look pretty good to me - I only have minor nits.

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

nullptr

@@ +691,5 @@
> +    // FIXME : see bug 973761, we need implement a mechanism that enable/disable hw codec on demand.
> +    CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> +                video_codec.plName, video_codec.plType);
> +    VideoDecoder* extDecoder = ExtVideoCodec::CreateDecoder();
> +//    SetExternalRecvCodec(video_codec.plType, extDecoder);

This is dead code now?

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +36,5 @@
> +
> +static const int64_t DEQUEUE_BUFFER_TIMEOUT_US = 100 * 1000ll; // 100ms
> +static const int64_t START_DEQUEUE_BUFFER_TIMEOUT_US = 10 * DEQUEUE_BUFFER_TIMEOUT_US; // 1s
> +static const char MEDIACODEC_VIDEO_MIME_VP8[] = "video/x-vnd.on2.vp8";
> +static const int32_t KINT32MAX = 0x7FFFFFFF;

INT32_MAX or use std::numeric_limits?

@@ +180,5 @@
> +  format->setInt32("height", codecSettings->height);
> +  format->setInt32("bitrate", codecSettings->startBitrate * 1000); // kbps->bps
> +//  format->setInt32("stride", codecSettings->width);
> +//  format->setInt32("slice-height", codecSettings->height);
> +//  format->setFloat("frame-rate", (float)codecSettings->maxFramerate);

You explained why this is commented out previously in this bug. I'd add a small note here or file the bug to fix it in bugzilla and mention the bug nr here.

@@ +216,5 @@
> +
> +    mOmxEncoder->isStarted = false;
> + }
> +
> +  // alloc buffer for storing encoded frame which format is 4:2:0 

nit: trailing whitespace

@@ +284,5 @@
> +#ifdef DEBUG
> +  time = PR_IntervalNow();
> +#endif
> +
> +  err = omx->queueInputBuffer(index, 0, size, inputImage.render_time_ms() * 1000 /* ms to us */, 0);

PR_USEC_PER_MSEC

@@ +405,5 @@
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +  if (!mOmxEncoder || !mOmxEncoder->mOmx.get()) {
> +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> +  }
> +// FIXME: implement this api once our device support it

File a bug and put bug nr?

@@ +528,5 @@
> +  memcpy(omxIn->data(), inputImage._buffer, size);
> +  if (firstTime < 0) {
> +    firstTime = inputImage._timeStamp;
> +  }
> +  *timeUs = (inputImage._timeStamp - firstTime) * 10; // FIXME

Fix what?

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8386182 [details] [diff] [review]
B969395_webrtc_hw_codec.v03.patch

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

r-, but it's a lot closer.  A number of questions to answer.  The only really bad thing is the encodedframe stuff.

::: media/webrtc/signaling/signaling.gyp
@@ +182,5 @@
>          'HAVE_UINT8_T=1',
>          'HAVE_UINT16_T=1',
>          'HAVE_UINT32_T=1',
>          'HAVE_UINT64_T=1',
> +        'HAVE_SYS_UIO_H',

We shouldn't be taking that as a given, I think.  That should come from config

@@ +226,5 @@
>            'cflags_mozilla': [
>            ],
> +
> +          'cflags': [
> +            '-Wno-multichar',

Two things: first that will only affect non-Mozilla code.  Second, why is this needed, and on what platforms?

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

remove nulling the pointer; none of the other Interface classes do it after Release().  Note that if bug 980304 lands first, just remove this entire block (it will handle Release())

@@ +895,5 @@
> +  CSFLogDebug(logTag, "%s: set External HW Send Codec.", __FUNCTION__);
> +  mPtrExtCodec->RegisterExternalSendCodec(mChannel,
> +                                          pltype,
> +                                          static_cast<
> +                                          WebrtcVideoEncoder*>(encoder), false);

go over 80 chars instead of breaking the cast list this

@@ +907,5 @@
> +  CSFLogDebug(logTag, "%s: set External HW Receive Codec.", __FUNCTION__);
> +  mPtrExtCodec->RegisterExternalReceiveCodec(mChannel,
> +                                             pltype,
> +                                             static_cast<
> +                                             WebrtcVideoDecoder*>(decoder));

ditto

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +12,5 @@
>  // Video Engine Includes
>  #include "webrtc/common_types.h"
> +
> +#ifdef FF
> +#undef FF

please at least comment why this is needed

@@ +283,5 @@
>    webrtc::ViECodec* mPtrViECodec;
>    webrtc::ViENetwork* mPtrViENetwork;
>    webrtc::ViERender* mPtrViERender;
>    webrtc::ViEExternalCapture*  mPtrExtCapture; // shared
> +  webrtc::ViEExternalCodec*  mPtrExtCodec;

Note: if bug 980304 lands first, this will need to be changed to a ScopedCustomReleasePtr<> (and the .Release() removed)

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +62,5 @@
> +    }
> +    if (mLooper != nullptr) {
> +      mLooper.clear();
> +    }
> +    if (mOmx != nullptr) {

for all of these, mozilla style is "if (ptr) { ... }"; don't compare to nullptr.

Please apply across the file/patch

@@ +83,5 @@
> +    if (mOmx != nullptr) {
> +      err = mOmx->configure(mMsg, nativeWindow, crypto, flags);
> +      CSFLogDebug(logTag, "WebrtcOMX::%s, err = %d", __FUNCTION__, err);
> +    }
> +    return err;

if (!mOmx) {
  mOmx = ....;
  if (!mOmx) {
    return BAD_VALUE;
  }
}
err = mOmx->configure(...);
CFSLogDebug(...);
return err;

@@ +91,5 @@
> +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +    status_t err = BAD_VALUE;
> +
> +    if (mOmx != nullptr) {
> +      status_t err = mOmx->start();

bug: this is aliasing the outer and won't get returned!

Suggest:
if (!mOmx) {
  return BAD_VALUE;
}
status_t err = mOmx->start();
...
etc

@@ +105,5 @@
> +  status_t Stop() {
> +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +
> +    status_t err = mOmx->stop();
> +    return err;

return mOmx->stop();

@@ +121,5 @@
> +  friend class WebrtcExtVideoEncoder;
> +  friend class WebrtcExtVideoDecoder;
> +
> +  sp<ALooper> mLooper;
> +  sp<MediaCodec> mOmx; // MediaCodec

comment is redundant

@@ +191,5 @@
> +  if (isEncoder) {
> +    format->setInt32("i-frame-interval", KINT32MAX); // should generate I-frame on demand
> +  }
> +  format->setInt32("profile", OMX_VIDEO_VP8ProfileMain);
> +  format->setInt32("level", OMX_VIDEO_VP8Level_Version3);

We'll need to make sure the higher layers have a way to know the max profile/level supported by a hardware encoder in order to negotiate (or to switch to a software codec in some cases, maybe).

Please file a bug to expose somewhere the HW codec config information to the outer code (SIPCC/Conduits)

@@ +221,5 @@
> +  size_t size = webrtc::CalcBufferSize(webrtc::kI420, codecSettings->width, codecSettings->height);
> +  if (mEncodedImage._size < size) {
> +    delete [] mEncodedImage._buffer;
> +    mEncodedImage._buffer = new uint8_t[size];
> +    mEncodedImage._size = size;

Still wrong.  The *encoded* buffer size can be *larger* than the source image. (Rare, but true: think a 16x16 or smaller input image.)

I suggest not pre-allocating a buffer here.  Instead, when you get an encoded image, if it's larger than the current buffer, reallocate the buffer to be big enough (perhaps round up to a 1K boundary to reduce churn).  See VCMEncodedFrame::VerifyAndAllocate(); that does the high-watermark thing I mention (with no rounding).

@@ +335,5 @@
> +
> +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> +  mEncodedImage._length = omxOut->size();
> +  uint8_t* data = omxOut->data();
> +  memcpy(mEncodedImage._buffer, data, mEncodedImage._length);

This is where we need to reallocate if needed.

@@ +371,5 @@
> +
> +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> +    webrtc::EncodedImageCallback* callback) {
> +  CSFLogDebug(logTag, "%s ", __FUNCTION__);
> +  mCallback = callback;

What thread does this happen on?  Does it happen on the thread where the encoder callback occur?  Can it happen on more than one thread?  Can it change during a stream of encode calls?  Is it only save to change between end of a previous callback and sending the next frame to be encoded? (assuming one frame "in-flight" at a time)

@@ +390,5 @@
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +WebrtcExtVideoEncoder::~WebrtcExtVideoEncoder() {
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);

should this call Release(), or MOZ_ASSERT() that those pointers are null?   (I.e. that Release was called)

@@ +405,5 @@
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +  if (!mOmxEncoder || !mOmxEncoder->mOmx.get()) {
> +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> +  }
> +// FIXME: implement this api once our device support it

ditto gcp's comment here, and also what is blocking this?  This is important, and I'm not sure it's really usable without it

@@ +471,5 @@
> +  uint8_t* src_nv12 = decoded->data();
> +
> +  int src_nv12_y_size = width * height;
> +  int src_nv12_uv_size = ((width + 1) / 2) * ((height + 1) / 2) * 2;
> +  int src_nv12_size = src_nv12_y_size + src_nv12_uv_size;

I suspect there are support macros for calculating these somewhere.  Not a big deal

@@ +498,5 @@
> +    const sp<MediaCodec>& omx,
> +    const webrtc::EncodedImage& inputImage,
> +    int64_t* timeUs) {
> +
> +  static int64_t firstTime = -1ll;

ll isn't necessary here, though it doesn't hurt.

@@ +506,5 @@
> +  uint32_t time = PR_IntervalNow();
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +#endif
> +
> +  status_t err = omx->dequeueInputBuffer(&index, firstTime < 0? START_DEQUEUE_BUFFER_TIMEOUT_US:DEQUEUE_BUFFER_TIMEOUT_US);

expr ? true_expr : false_expr
(i.e. whitespace)  Please apply throughout file

Also, wrap it please

@@ +524,5 @@
> +  }
> +  size_t size = inputImage._length;
> +  const sp<ABuffer>& omxIn = decoder->getInput()->itemAt(index);
> +  omxIn->setRange(0, size);
> +  memcpy(omxIn->data(), inputImage._buffer, size);

It's too bad the API forces us to memcpy this....  Is there any way we might be able to 'hold' the buffer or transfer it from inputImage (ping-pong buffers or select them from a small pool)

@@ +528,5 @@
> +  memcpy(omxIn->data(), inputImage._buffer, size);
> +  if (firstTime < 0) {
> +    firstTime = inputImage._timeStamp;
> +  }
> +  *timeUs = (inputImage._timeStamp - firstTime) * 10; // FIXME

ditto gcp's comment, and also how is * 10 making this into microseconds?  or is it a fudge factor?

@@ +552,5 @@
> +
> +  CSFLogDebug(logTag,  "%s , encodedWidth = %d, encodedHeight = %d",
> +              __FUNCTION__, inputImage._encodedWidth, inputImage._encodedHeight);
> +
> +  EncodedFrame* frame = new EncodedFrame();

How about a ScopedDeletePtr<EncodedFrame> frame, and then get rid of the delete frame and goto end's?
And/Or just move it down to right before GenerateVideoFrame().

@@ +593,5 @@
> +    // invalid buffer
> +    omx->releaseOutputBuffer(index);
> +  } else {
> +    CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() generate video frame", __FUNCTION__);
> +    GenerateVideoFrame(frame, omxOut, video_frame);

what exactly does 'frame' do here anyways?

Comment 32

[qiang lu]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #30)
> Comment on attachment 8386182 [details] [diff] [review]
> B969395_webrtc_hw_codec.v03.patch
> 
> Review of attachment 8386182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ...and Bugzilla lost my initial review comments :(
> 
> The code is starting to look pretty good to me - I only have minor nits.
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +68,5 @@
> >    delete mCurSendCodecConfig;
> >  
> > +  if (mPtrExtCodec) {
> > +    mPtrExtCodec->Release();
> > +    mPtrExtCodec = NULL;
> 
> nullptr
Done.

> 
> @@ +691,5 @@
> > +    // FIXME : see bug 973761, we need implement a mechanism that enable/disable hw codec on demand.
> > +    CSFLogDebug(logTag, "%s, in ExtVideoCodec codec %s(%d)", __FUNCTION__,
> > +                video_codec.plName, video_codec.plType);
> > +    VideoDecoder* extDecoder = ExtVideoCodec::CreateDecoder();
> > +//    SetExternalRecvCodec(video_codec.plType, extDecoder);
> 
> This is dead code now?
> 
Done, uncomment it (i comment it for debugging decode module but forget enable it before generate patch)

> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +36,5 @@
> > +
> > +static const int64_t DEQUEUE_BUFFER_TIMEOUT_US = 100 * 1000ll; // 100ms
> > +static const int64_t START_DEQUEUE_BUFFER_TIMEOUT_US = 10 * DEQUEUE_BUFFER_TIMEOUT_US; // 1s
> > +static const char MEDIACODEC_VIDEO_MIME_VP8[] = "video/x-vnd.on2.vp8";
> > +static const int32_t KINT32MAX = 0x7FFFFFFF;
> 
> INT32_MAX or use std::numeric_limits?
> 
Done. using std::numeric_limits<int>::max() instead.

> @@ +180,5 @@
> > +  format->setInt32("height", codecSettings->height);
> > +  format->setInt32("bitrate", codecSettings->startBitrate * 1000); // kbps->bps
> > +//  format->setInt32("stride", codecSettings->width);
> > +//  format->setInt32("slice-height", codecSettings->height);
> > +//  format->setFloat("frame-rate", (float)codecSettings->maxFramerate);
> 
> You explained why this is commented out previously in this bug. I'd add a
> small note here or file the bug to fix it in bugzilla and mention the bug nr
> here.

I double check the internal implementation of mediacodec and it turns out that those commented setting could be set by mediacodec native api. so i remove them from this patch.

> 
> @@ +216,5 @@
> > +
> > +    mOmxEncoder->isStarted = false;
> > + }
> > +
> > +  // alloc buffer for storing encoded frame which format is 4:2:0 
> 
> nit: trailing whitespace
Done.

> 
> @@ +284,5 @@
> > +#ifdef DEBUG
> > +  time = PR_IntervalNow();
> > +#endif
> > +
> > +  err = omx->queueInputBuffer(index, 0, size, inputImage.render_time_ms() * 1000 /* ms to us */, 0);
> 
> PR_USEC_PER_MSEC
Done.

> 
> @@ +405,5 @@
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +  if (!mOmxEncoder || !mOmxEncoder->mOmx.get()) {
> > +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> > +  }
> > +// FIXME: implement this api once our device support it
> 
> File a bug and put bug nr?
> 
Done. remove this FIXME and enable this fundamental function.

> @@ +528,5 @@
> > +  memcpy(omxIn->data(), inputImage._buffer, size);
> > +  if (firstTime < 0) {
> > +    firstTime = inputImage._timeStamp;
> > +  }
> > +  *timeUs = (inputImage._timeStamp - firstTime) * 10; // FIXME
> 
> Fix what?
Done. Try to use incoming renderTimeUs as the input parameter of queueInputBuffer().

Comment 33

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #31)
> Comment on attachment 8386182 [details] [diff] [review]
> B969395_webrtc_hw_codec.v03.patch
> 
> Review of attachment 8386182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-, but it's a lot closer.  A number of questions to answer.  The only
> really bad thing is the encodedframe stuff.
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +182,5 @@
> >          'HAVE_UINT8_T=1',
> >          'HAVE_UINT16_T=1',
> >          'HAVE_UINT32_T=1',
> >          'HAVE_UINT64_T=1',
> > +        'HAVE_SYS_UIO_H',
> 
> We shouldn't be taking that as a given, I think.  That should come from
> config

Could you please guide me how to move this Flag into config ?

> 
> @@ +226,5 @@
> >            'cflags_mozilla': [
> >            ],
> > +
> > +          'cflags': [
> > +            '-Wno-multichar',
> 
> Two things: first that will only affect non-Mozilla code.  Second, why is
> this needed, and on what platforms?
Done. move it to 'cflags_mozilla' which is used to ignore "-Wmultichar" compile warning since /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/omx-plugin/include/kk/media/stagefright/MediaCodec.h is touched by WebrtcExtVideoCodec.cpp

see some compile warnings if don't include -Wno-multichar flag
 0:08.83 In file included from /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/../trunk/webrtc/modules/interface/module_common_types.h:20:0,
 0:08.83                  from /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/../trunk/webrtc/modules/video_coding/codecs/interface/video_codec_interface.h:19,
 0:08.83                  from /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/src/media-conduit/VideoConduit.h:22,
 0:08.83                  from /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h:26,
 0:08.83                  from /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:12:
 0:08.83 /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/../trunk/webrtc/system_wrappers/interface/constructor_magic.h:34:0: note: this is the location of the previous definition
 0:08.83 In file included from /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:27:0:
 0:08.83 Warning: -Wmultichar in /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/omx-plugin/include/kk/media/stagefright/MediaCodec.h: multi-character character constant
 0:08.83 /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/media/stagefright/MediaCodec.h:148:21: warning: multi-character character constant [-Wmultichar]
 0:08.83 Warning: -Wmultichar in /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/omx-plugin/include/kk/media/stagefright/MediaCodec.h: multi-character character constant
 0:08.83 /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/../../../media/omx-plugin/include/kk/media/stagefright/MediaCodec.h:149:26: warning: multi-character character constant [-Wmultichar]
....

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +68,5 @@
> >    delete mCurSendCodecConfig;
> >  
> > +  if (mPtrExtCodec) {
> > +    mPtrExtCodec->Release();
> > +    mPtrExtCodec = NULL;
> 
> remove nulling the pointer; none of the other Interface classes do it after
> Release().  Note that if bug 980304 lands first, just remove this entire
> block (it will handle Release())

Done. if B980304 landed, i will back here to remove those code.

> 
> @@ +895,5 @@
> > +  CSFLogDebug(logTag, "%s: set External HW Send Codec.", __FUNCTION__);
> > +  mPtrExtCodec->RegisterExternalSendCodec(mChannel,
> > +                                          pltype,
> > +                                          static_cast<
> > +                                          WebrtcVideoEncoder*>(encoder), false);
> 
> go over 80 chars instead of breaking the cast list this
> 
Done.

> @@ +907,5 @@
> > +  CSFLogDebug(logTag, "%s: set External HW Receive Codec.", __FUNCTION__);
> > +  mPtrExtCodec->RegisterExternalReceiveCodec(mChannel,
> > +                                             pltype,
> > +                                             static_cast<
> > +                                             WebrtcVideoDecoder*>(decoder));
> 
> ditto
> 
Done.

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +12,5 @@
> >  // Video Engine Includes
> >  #include "webrtc/common_types.h"
> > +
> > +#ifdef FF
> > +#undef FF
> 
> please at least comment why this is needed
Done. Add comment.

> 
> @@ +283,5 @@
> >    webrtc::ViECodec* mPtrViECodec;
> >    webrtc::ViENetwork* mPtrViENetwork;
> >    webrtc::ViERender* mPtrViERender;
> >    webrtc::ViEExternalCapture*  mPtrExtCapture; // shared
> > +  webrtc::ViEExternalCodec*  mPtrExtCodec;
> 
> Note: if bug 980304 lands first, this will need to be changed to a
> ScopedCustomReleasePtr<> (and the .Release() removed)
Wait for B980304 lands

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +62,5 @@
> > +    }
> > +    if (mLooper != nullptr) {
> > +      mLooper.clear();
> > +    }
> > +    if (mOmx != nullptr) {
> 
> for all of these, mozilla style is "if (ptr) { ... }"; don't compare to
> nullptr.
> 
> Please apply across the file/patch
I can't check mLooper/mOmx/mMsg whether is nullptr by if(ptr) since they are sp<> point.
If i do check like normal mozilla style, below compile error were generated:

 0:14.76 /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:60:13: error: could not convert '((mozilla::WebrtcOMX*)this)->mozilla::WebrtcOMX::mMsg' from 'android::sp<android::AMessage>' to 'bool'


> 
> @@ +83,5 @@
> > +    if (mOmx != nullptr) {
> > +      err = mOmx->configure(mMsg, nativeWindow, crypto, flags);
> > +      CSFLogDebug(logTag, "WebrtcOMX::%s, err = %d", __FUNCTION__, err);
> > +    }
> > +    return err;
> 
> if (!mOmx) {
>   mOmx = ....;
>   if (!mOmx) {
>     return BAD_VALUE;
>   }
> }
> err = mOmx->configure(...);
> CFSLogDebug(...);
> return err;
> 
Done.

> @@ +91,5 @@
> > +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +    status_t err = BAD_VALUE;
> > +
> > +    if (mOmx != nullptr) {
> > +      status_t err = mOmx->start();
> 
> bug: this is aliasing the outer and won't get returned!
> 
> Suggest:
> if (!mOmx) {
>   return BAD_VALUE;
> }
> status_t err = mOmx->start();
> ...
> etc

Done.

> 
> @@ +105,5 @@
> > +  status_t Stop() {
> > +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +
> > +    status_t err = mOmx->stop();
> > +    return err;
> 
> return mOmx->stop();

Done.

> 
> @@ +121,5 @@
> > +  friend class WebrtcExtVideoEncoder;
> > +  friend class WebrtcExtVideoDecoder;
> > +
> > +  sp<ALooper> mLooper;
> > +  sp<MediaCodec> mOmx; // MediaCodec
> 
> comment is redundant
> 
Done. remove comment.

> @@ +191,5 @@
> > +  if (isEncoder) {
> > +    format->setInt32("i-frame-interval", KINT32MAX); // should generate I-frame on demand
> > +  }
> > +  format->setInt32("profile", OMX_VIDEO_VP8ProfileMain);
> > +  format->setInt32("level", OMX_VIDEO_VP8Level_Version3);
> 
> We'll need to make sure the higher layers have a way to know the max
> profile/level supported by a hardware encoder in order to negotiate (or to
> switch to a software codec in some cases, maybe).
AFAIK, we could get the max profile/level supported by a hardware codec from mediacodec api. It is not exposed to libstagefright framework right now.

> 
> Please file a bug to expose somewhere the HW codec config information to the
> outer code (SIPCC/Conduits)

I will file a bug to expose those HW codec config information(CodecType, width, height, color format, framerate) to up layer(SIPCC/Conduits).

> 
> @@ +221,5 @@
> > +  size_t size = webrtc::CalcBufferSize(webrtc::kI420, codecSettings->width, codecSettings->height);
> > +  if (mEncodedImage._size < size) {
> > +    delete [] mEncodedImage._buffer;
> > +    mEncodedImage._buffer = new uint8_t[size];
> > +    mEncodedImage._size = size;
> 
> Still wrong.  The *encoded* buffer size can be *larger* than the source
> image. (Rare, but true: think a 16x16 or smaller input image.)
Got it. Thanks for your explanation.

> 
> I suggest not pre-allocating a buffer here.  Instead, when you get an
> encoded image, if it's larger than the current buffer, reallocate the buffer
> to be big enough (perhaps round up to a 1K boundary to reduce churn).  See
> VCMEncodedFrame::VerifyAndAllocate(); that does the high-watermark thing I
> mention (with no rounding).
Done as your mentioned.

> 
> @@ +335,5 @@
> > +
> > +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> > +  mEncodedImage._length = omxOut->size();
> > +  uint8_t* data = omxOut->data();
> > +  memcpy(mEncodedImage._buffer, data, mEncodedImage._length);
> 
> This is where we need to reallocate if needed.
> 
> @@ +371,5 @@
> > +
> > +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> > +    webrtc::EncodedImageCallback* callback) {
> > +  CSFLogDebug(logTag, "%s ", __FUNCTION__);
> > +  mCallback = callback;
> 
> What thread does this happen on?  Does it happen on the thread where the
> encoder callback occur?  Can it happen on more than one thread?  Can it
> change during a stream of encode calls?  Is it only save to change between
> end of a previous callback and sending the next frame to be encoded?
> (assuming one frame "in-flight" at a time)

From my experience, it called by main webrtc thread which is thread where encoder callback occur.
I don't think it can happen on more than one thread and couldn't be changed during a stream of encode calls.
I will keep those things your mentioned in head during the next work.

> 
> @@ +390,5 @@
> > +  return WEBRTC_VIDEO_CODEC_OK;
> > +}
> > +
> > +WebrtcExtVideoEncoder::~WebrtcExtVideoEncoder() {
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> 
> should this call Release(), or MOZ_ASSERT() that those pointers are null?  
> (I.e. that Release was called)
Done. call Release() in the destructor function of WebrtcExtVideoEncoder and WebrtcExtVideoDecoder

> 
> @@ +405,5 @@
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +  if (!mOmxEncoder || !mOmxEncoder->mOmx.get()) {
> > +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> > +  }
> > +// FIXME: implement this api once our device support it
> 
> ditto gcp's comment here, and also what is blocking this?  This is
> important, and I'm not sure it's really usable without it

Done. Remove FIXME and enable this fundamental API.

> 
> @@ +471,5 @@
> > +  uint8_t* src_nv12 = decoded->data();
> > +
> > +  int src_nv12_y_size = width * height;
> > +  int src_nv12_uv_size = ((width + 1) / 2) * ((height + 1) / 2) * 2;
> > +  int src_nv12_size = src_nv12_y_size + src_nv12_uv_size;
> 
> I suspect there are support macros for calculating these somewhere.  Not a
> big deal

Not change. (I don't find usable macros in libyuv so ...)

> 
> @@ +498,5 @@
> > +    const sp<MediaCodec>& omx,
> > +    const webrtc::EncodedImage& inputImage,
> > +    int64_t* timeUs) {
> > +
> > +  static int64_t firstTime = -1ll;
> 
> ll isn't necessary here, though it doesn't hurt.
Done. remove ll.

> 
> @@ +506,5 @@
> > +  uint32_t time = PR_IntervalNow();
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +#endif
> > +
> > +  status_t err = omx->dequeueInputBuffer(&index, firstTime < 0? START_DEQUEUE_BUFFER_TIMEOUT_US:DEQUEUE_BUFFER_TIMEOUT_US);
> 
> expr ? true_expr : false_expr
> (i.e. whitespace)  Please apply throughout file
> 
> Also, wrap it please
Done.

> 
> @@ +524,5 @@
> > +  }
> > +  size_t size = inputImage._length;
> > +  const sp<ABuffer>& omxIn = decoder->getInput()->itemAt(index);
> > +  omxIn->setRange(0, size);
> > +  memcpy(omxIn->data(), inputImage._buffer, size);
> 
> It's too bad the API forces us to memcpy this....  Is there any way we might
> be able to 'hold' the buffer or transfer it from inputImage (ping-pong
> buffers or select them from a small pool)

Not done. 

I will try to do what your suggestion and keep it in this patch temporally. 

> 
> @@ +528,5 @@
> > +  memcpy(omxIn->data(), inputImage._buffer, size);
> > +  if (firstTime < 0) {
> > +    firstTime = inputImage._timeStamp;
> > +  }
> > +  *timeUs = (inputImage._timeStamp - firstTime) * 10; // FIXME
> 
> ditto gcp's comment, and also how is * 10 making this into microseconds?  or
> is it a fudge factor?
> 
Done. remove these test code. using renderTimeUs instead timeUs.

> @@ +552,5 @@
> > +
> > +  CSFLogDebug(logTag,  "%s , encodedWidth = %d, encodedHeight = %d",
> > +              __FUNCTION__, inputImage._encodedWidth, inputImage._encodedHeight);
> > +
> > +  EncodedFrame* frame = new EncodedFrame();
> 
> How about a ScopedDeletePtr<EncodedFrame> frame, and then get rid of the
> delete frame and goto end's?
> And/Or just move it down to right before GenerateVideoFrame().
> 
Done. remove frame without side-effect.

> @@ +593,5 @@
> > +    // invalid buffer
> > +    omx->releaseOutputBuffer(index);
> > +  } else {
> > +    CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() generate video frame", __FUNCTION__);
> > +    GenerateVideoFrame(frame, omxOut, video_frame);
> 
> what exactly does 'frame' do here anyways?

Done. remove the using of frame from this patch.

Comment 34

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v04.patch

Comment 35

[qiang lu]

(In reply to qiang lu from comment #33)
> (In reply to Randell Jesup [:jesup] from comment #31)
> > Comment on attachment 8386182 [details] [diff] [review]
> > B969395_webrtc_hw_codec.v03.patch
> > 
> > Review of attachment 8386182 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r-, but it's a lot closer.  A number of questions to answer.  The only
> > really bad thing is the encodedframe stuff.
> > 
> > ::: media/webrtc/signaling/signaling.gyp
> > @@ +182,5 @@
> > >          'HAVE_UINT8_T=1',
> > >          'HAVE_UINT16_T=1',
> > >          'HAVE_UINT32_T=1',
> > >          'HAVE_UINT64_T=1',
> > > +        'HAVE_SYS_UIO_H',
> > 
> > We shouldn't be taking that as a given, I think.  That should come from
> > config
> 
> Could you please guide me how to move this Flag into config ?
> 
> > 
> > @@ +226,5 @@
> > >            'cflags_mozilla': [
> > >            ],
> > > +
> > > +          'cflags': [
> > > +            '-Wno-multichar',
> > 
> > Two things: first that will only affect non-Mozilla code.  Second, why is
> > this needed, and on what platforms?
> Done. move it to 'cflags_mozilla' which is used to ignore "-Wmultichar"
> compile warning since
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/omx-plugin/
> include/kk/media/stagefright/MediaCodec.h is touched by
> WebrtcExtVideoCodec.cpp
> 
> see some compile warnings if don't include -Wno-multichar flag
>  0:08.83 In file included from
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/../trunk/webrtc/modules/interface/module_common_types.h:20:0,
>  0:08.83                  from
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/../trunk/webrtc/modules/video_coding/codecs/interface/
> video_codec_interface.h:19,
>  0:08.83                  from
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/src/media-conduit/VideoConduit.h:22,
>  0:08.83                  from
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/src/media-conduit/WebrtcExtVideoCodec.h:26,
>  0:08.83                  from
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:12:
>  0:08.83
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/../trunk/webrtc/system_wrappers/interface/constructor_magic.h:34:0:
> note: this is the location of the previous definition
>  0:08.83 In file included from
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:27:0:
>  0:08.83 Warning: -Wmultichar in
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/omx-plugin/
> include/kk/media/stagefright/MediaCodec.h: multi-character character constant
>  0:08.83
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/../../../media/omx-plugin/include/kk/media/stagefright/MediaCodec.
> h:148:21: warning: multi-character character constant [-Wmultichar]
>  0:08.83 Warning: -Wmultichar in
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/omx-plugin/
> include/kk/media/stagefright/MediaCodec.h: multi-character character constant
>  0:08.83
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/../../../media/omx-plugin/include/kk/media/stagefright/MediaCodec.
> h:149:26: warning: multi-character character constant [-Wmultichar]
> ....
> 
> > 
> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > @@ +68,5 @@
> > >    delete mCurSendCodecConfig;
> > >  
> > > +  if (mPtrExtCodec) {
> > > +    mPtrExtCodec->Release();
> > > +    mPtrExtCodec = NULL;
> > 
> > remove nulling the pointer; none of the other Interface classes do it after
> > Release().  Note that if bug 980304 lands first, just remove this entire
> > block (it will handle Release())
> 
> Done. if B980304 landed, i will back here to remove those code.
> 
> > 
> > @@ +895,5 @@
> > > +  CSFLogDebug(logTag, "%s: set External HW Send Codec.", __FUNCTION__);
> > > +  mPtrExtCodec->RegisterExternalSendCodec(mChannel,
> > > +                                          pltype,
> > > +                                          static_cast<
> > > +                                          WebrtcVideoEncoder*>(encoder), false);
> > 
> > go over 80 chars instead of breaking the cast list this
> > 
> Done.
> 
> > @@ +907,5 @@
> > > +  CSFLogDebug(logTag, "%s: set External HW Receive Codec.", __FUNCTION__);
> > > +  mPtrExtCodec->RegisterExternalReceiveCodec(mChannel,
> > > +                                             pltype,
> > > +                                             static_cast<
> > > +                                             WebrtcVideoDecoder*>(decoder));
> > 
> > ditto
> > 
> Done.
> 
> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> > @@ +12,5 @@
> > >  // Video Engine Includes
> > >  #include "webrtc/common_types.h"
> > > +
> > > +#ifdef FF
> > > +#undef FF
> > 
> > please at least comment why this is needed
> Done. Add comment.
> 
> > 
> > @@ +283,5 @@
> > >    webrtc::ViECodec* mPtrViECodec;
> > >    webrtc::ViENetwork* mPtrViENetwork;
> > >    webrtc::ViERender* mPtrViERender;
> > >    webrtc::ViEExternalCapture*  mPtrExtCapture; // shared
> > > +  webrtc::ViEExternalCodec*  mPtrExtCodec;
> > 
> > Note: if bug 980304 lands first, this will need to be changed to a
> > ScopedCustomReleasePtr<> (and the .Release() removed)
> Wait for B980304 lands
> 
> > 
> > ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> > @@ +62,5 @@
> > > +    }
> > > +    if (mLooper != nullptr) {
> > > +      mLooper.clear();
> > > +    }
> > > +    if (mOmx != nullptr) {
> > 
> > for all of these, mozilla style is "if (ptr) { ... }"; don't compare to
> > nullptr.
> > 
> > Please apply across the file/patch
> I can't check mLooper/mOmx/mMsg whether is nullptr by if(ptr) since they are
> sp<> point.
> If i do check like normal mozilla style, below compile error were generated:
> 
>  0:14.76
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:60:13: error: could not
> convert '((mozilla::WebrtcOMX*)this)->mozilla::WebrtcOMX::mMsg' from
> 'android::sp<android::AMessage>' to 'bool'
> 
> 
> > 
> > @@ +83,5 @@
> > > +    if (mOmx != nullptr) {
> > > +      err = mOmx->configure(mMsg, nativeWindow, crypto, flags);
> > > +      CSFLogDebug(logTag, "WebrtcOMX::%s, err = %d", __FUNCTION__, err);
> > > +    }
> > > +    return err;
> > 
> > if (!mOmx) {
> >   mOmx = ....;
> >   if (!mOmx) {
> >     return BAD_VALUE;
> >   }
> > }
> > err = mOmx->configure(...);
> > CFSLogDebug(...);
> > return err;
> > 
> Done.
> 
> > @@ +91,5 @@
> > > +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > > +    status_t err = BAD_VALUE;
> > > +
> > > +    if (mOmx != nullptr) {
> > > +      status_t err = mOmx->start();
> > 
> > bug: this is aliasing the outer and won't get returned!
> > 
> > Suggest:
> > if (!mOmx) {
> >   return BAD_VALUE;
> > }
> > status_t err = mOmx->start();
> > ...
> > etc
> 
> Done.
> 
> > 
> > @@ +105,5 @@
> > > +  status_t Stop() {
> > > +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > > +
> > > +    status_t err = mOmx->stop();
> > > +    return err;
> > 
> > return mOmx->stop();
> 
> Done.
> 
> > 
> > @@ +121,5 @@
> > > +  friend class WebrtcExtVideoEncoder;
> > > +  friend class WebrtcExtVideoDecoder;
> > > +
> > > +  sp<ALooper> mLooper;
> > > +  sp<MediaCodec> mOmx; // MediaCodec
> > 
> > comment is redundant
> > 
> Done. remove comment.
> 
> > @@ +191,5 @@
> > > +  if (isEncoder) {
> > > +    format->setInt32("i-frame-interval", KINT32MAX); // should generate I-frame on demand
> > > +  }
> > > +  format->setInt32("profile", OMX_VIDEO_VP8ProfileMain);
> > > +  format->setInt32("level", OMX_VIDEO_VP8Level_Version3);
> > 
> > We'll need to make sure the higher layers have a way to know the max
> > profile/level supported by a hardware encoder in order to negotiate (or to
> > switch to a software codec in some cases, maybe).
> AFAIK, we could get the max profile/level supported by a hardware codec from
> mediacodec api. It is not exposed to libstagefright framework right now.
> 
> > 
> > Please file a bug to expose somewhere the HW codec config information to the
> > outer code (SIPCC/Conduits)
> 
> I will file a bug to expose those HW codec config information(CodecType,
> width, height, color format, framerate) to up layer(SIPCC/Conduits).
> 
> > 
> > @@ +221,5 @@
> > > +  size_t size = webrtc::CalcBufferSize(webrtc::kI420, codecSettings->width, codecSettings->height);
> > > +  if (mEncodedImage._size < size) {
> > > +    delete [] mEncodedImage._buffer;
> > > +    mEncodedImage._buffer = new uint8_t[size];
> > > +    mEncodedImage._size = size;
> > 
> > Still wrong.  The *encoded* buffer size can be *larger* than the source
> > image. (Rare, but true: think a 16x16 or smaller input image.)
> Got it. Thanks for your explanation.
> 
> > 
> > I suggest not pre-allocating a buffer here.  Instead, when you get an
> > encoded image, if it's larger than the current buffer, reallocate the buffer
> > to be big enough (perhaps round up to a 1K boundary to reduce churn).  See
> > VCMEncodedFrame::VerifyAndAllocate(); that does the high-watermark thing I
> > mention (with no rounding).
> Done as your mentioned.
> 
> > 
> > @@ +335,5 @@
> > > +
> > > +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> > > +  mEncodedImage._length = omxOut->size();
> > > +  uint8_t* data = omxOut->data();
> > > +  memcpy(mEncodedImage._buffer, data, mEncodedImage._length);
> > 
> > This is where we need to reallocate if needed.
> > 
> > @@ +371,5 @@
> > > +
> > > +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> > > +    webrtc::EncodedImageCallback* callback) {
> > > +  CSFLogDebug(logTag, "%s ", __FUNCTION__);
> > > +  mCallback = callback;
> > 
> > What thread does this happen on?  Does it happen on the thread where the
> > encoder callback occur?  Can it happen on more than one thread?  Can it
> > change during a stream of encode calls?  Is it only save to change between
> > end of a previous callback and sending the next frame to be encoded?
> > (assuming one frame "in-flight" at a time)
> 
> From my experience, it called by main webrtc thread which is thread where
> encoder callback occur.
> I don't think it can happen on more than one thread and couldn't be changed
> during a stream of encode calls.
> I will keep those things your mentioned in head during the next work.
> 
> > 
> > @@ +390,5 @@
> > > +  return WEBRTC_VIDEO_CODEC_OK;
> > > +}
> > > +
> > > +WebrtcExtVideoEncoder::~WebrtcExtVideoEncoder() {
> > > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > 
> > should this call Release(), or MOZ_ASSERT() that those pointers are null?  
> > (I.e. that Release was called)
> Done. call Release() in the destructor function of WebrtcExtVideoEncoder and
> WebrtcExtVideoDecoder
> 
> > 
> > @@ +405,5 @@
> > > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > > +  if (!mOmxEncoder || !mOmxEncoder->mOmx.get()) {
> > > +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> > > +  }
> > > +// FIXME: implement this api once our device support it
> > 
> > ditto gcp's comment here, and also what is blocking this?  This is
> > important, and I'm not sure it's really usable without it
> 
> Done. Remove FIXME and enable this fundamental API.
> 
> > 
> > @@ +471,5 @@
> > > +  uint8_t* src_nv12 = decoded->data();
> > > +
> > > +  int src_nv12_y_size = width * height;
> > > +  int src_nv12_uv_size = ((width + 1) / 2) * ((height + 1) / 2) * 2;
> > > +  int src_nv12_size = src_nv12_y_size + src_nv12_uv_size;
> > 
> > I suspect there are support macros for calculating these somewhere.  Not a
> > big deal
> 
> Not change. (I don't find usable macros in libyuv so ...)
> 
> > 
> > @@ +498,5 @@
> > > +    const sp<MediaCodec>& omx,
> > > +    const webrtc::EncodedImage& inputImage,
> > > +    int64_t* timeUs) {
> > > +
> > > +  static int64_t firstTime = -1ll;
> > 
> > ll isn't necessary here, though it doesn't hurt.
> Done. remove ll.
> 
> > 
> > @@ +506,5 @@
> > > +  uint32_t time = PR_IntervalNow();
> > > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > > +#endif
> > > +
> > > +  status_t err = omx->dequeueInputBuffer(&index, firstTime < 0? START_DEQUEUE_BUFFER_TIMEOUT_US:DEQUEUE_BUFFER_TIMEOUT_US);
> > 
> > expr ? true_expr : false_expr
> > (i.e. whitespace)  Please apply throughout file
> > 
> > Also, wrap it please
> Done.
> 
> > 
> > @@ +524,5 @@
> > > +  }
> > > +  size_t size = inputImage._length;
> > > +  const sp<ABuffer>& omxIn = decoder->getInput()->itemAt(index);
> > > +  omxIn->setRange(0, size);
> > > +  memcpy(omxIn->data(), inputImage._buffer, size);
> > 
> > It's too bad the API forces us to memcpy this....  Is there any way we might
> > be able to 'hold' the buffer or transfer it from inputImage (ping-pong
> > buffers or select them from a small pool)
> 
> Not done. 
> 
> I will try to do what your suggestion and keep it in this patch temporally. 
> 
> > 
> > @@ +528,5 @@
> > > +  memcpy(omxIn->data(), inputImage._buffer, size);
> > > +  if (firstTime < 0) {
> > > +    firstTime = inputImage._timeStamp;
> > > +  }
> > > +  *timeUs = (inputImage._timeStamp - firstTime) * 10; // FIXME
> > 
> > ditto gcp's comment, and also how is * 10 making this into microseconds?  or
> > is it a fudge factor?
> > 
> Done. remove these test code. using renderTimeUs instead timeUs.
> 
> > @@ +552,5 @@
> > > +
> > > +  CSFLogDebug(logTag,  "%s , encodedWidth = %d, encodedHeight = %d",
> > > +              __FUNCTION__, inputImage._encodedWidth, inputImage._encodedHeight);
> > > +
> > > +  EncodedFrame* frame = new EncodedFrame();
> > 
> > How about a ScopedDeletePtr<EncodedFrame> frame, and then get rid of the
> > delete frame and goto end's?
> > And/Or just move it down to right before GenerateVideoFrame().
> > 
> Done. remove frame without side-effect.
> 
> > @@ +593,5 @@
> > > +    // invalid buffer
> > > +    omx->releaseOutputBuffer(index);
> > > +  } else {
> > > +    CSFLogDebug(logTag,  "%s WebrtcExtVideoDecoder::Decode() generate video frame", __FUNCTION__);
> > > +    GenerateVideoFrame(frame, omxOut, video_frame);
> > 
> > what exactly does 'frame' do here anyways?
> 
> Done. remove the using of frame from this patch.

And, there are MediaCodecInfo API for getting Codec support information on device, but it doesn't tell us which codec is supported by HW acceleration or SW.

Comment 36

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v04_01.patch

minor update

Comment 37

[qiang lu]

Hi gcp and rejesup,

Could you please help to review this updated patch and B973761?

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

(In reply to qiang lu from comment #33)

> > ::: media/webrtc/signaling/signaling.gyp
> > @@ +182,5 @@
> > >          'HAVE_UINT8_T=1',
> > >          'HAVE_UINT16_T=1',
> > >          'HAVE_UINT32_T=1',
> > >          'HAVE_UINT64_T=1',
> > > +        'HAVE_SYS_UIO_H',
> > 
> > We shouldn't be taking that as a given, I think.  That should come from
> > config
> 
> Could you please guide me how to move this Flag into config ?

Add it to a line like:
MOZ_CHECK_HEADERS(sys/types.h netinet/in.h byteswap.h)

in the "Checks for header files" section of configure.in


> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > @@ +68,5 @@
> > >    delete mCurSendCodecConfig;
> > >  
> > > +  if (mPtrExtCodec) {
> > > +    mPtrExtCodec->Release();
> > > +    mPtrExtCodec = NULL;
> > 
> > remove nulling the pointer; none of the other Interface classes do it after
> > Release().  Note that if bug 980304 lands first, just remove this entire
> > block (it will handle Release())
> 
> Done. if B980304 landed, i will back here to remove those code.

That bug has landed, so you'll need to update this.


> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> > @@ +283,5 @@
> > >    webrtc::ViECodec* mPtrViECodec;
> > >    webrtc::ViENetwork* mPtrViENetwork;
> > >    webrtc::ViERender* mPtrViERender;
> > >    webrtc::ViEExternalCapture*  mPtrExtCapture; // shared
> > > +  webrtc::ViEExternalCodec*  mPtrExtCodec;
> > 
> > Note: if bug 980304 lands first, this will need to be changed to a
> > ScopedCustomReleasePtr<> (and the .Release() removed)
> Wait for B980304 lands

Ditto


> > ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> > @@ +62,5 @@
> > > +    }
> > > +    if (mLooper != nullptr) {
> > > +      mLooper.clear();
> > > +    }
> > > +    if (mOmx != nullptr) {
> > 
> > for all of these, mozilla style is "if (ptr) { ... }"; don't compare to
> > nullptr.
> > 
> > Please apply across the file/patch
> I can't check mLooper/mOmx/mMsg whether is nullptr by if(ptr) since they are
> sp<> point.
> If i do check like normal mozilla style, below compile error were generated:
> 
>  0:14.76
> /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:60:13: error: could not
> convert '((mozilla::WebrtcOMX*)this)->mozilla::WebrtcOMX::mMsg' from
> 'android::sp<android::AMessage>' to 'bool'

Ok.


> > @@ +371,5 @@
> > > +
> > > +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> > > +    webrtc::EncodedImageCallback* callback) {
> > > +  CSFLogDebug(logTag, "%s ", __FUNCTION__);
> > > +  mCallback = callback;
> > 
> > What thread does this happen on?  Does it happen on the thread where the
> > encoder callback occur?  Can it happen on more than one thread?  Can it
> > change during a stream of encode calls?  Is it only save to change between
> > end of a previous callback and sending the next frame to be encoded?
> > (assuming one frame "in-flight" at a time)
> 
> From my experience, it called by main webrtc thread which is thread where
> encoder callback occur.

True; I suspect the webrtc encoder will call you back from one thread for a given instance of webrtc.  

Note that two instances of webrtc could result in two encoder threads (in different VideoEngine contexts).  For reference, if two separate threads try to use the OMX encoder, what happens?  Can the hardware support multiple encoder instances actively in use at the same time? (I presume one encode request would block if the HW was in use if this is supported.)  Or will only one of those get the hardware encoder, and the other get software OMX, or will one get HW and the other fail (and fall back to SW encoder in Webrtc)?


> I don't think it can happen on more than one thread and couldn't be changed
> during a stream of encode calls.

For a given stream, I believe this is correct (see above).

> I will keep those things your mentioned in head during the next work.


> > @@ +471,5 @@
> > > +  uint8_t* src_nv12 = decoded->data();
> > > +
> > > +  int src_nv12_y_size = width * height;
> > > +  int src_nv12_uv_size = ((width + 1) / 2) * ((height + 1) / 2) * 2;
> > > +  int src_nv12_size = src_nv12_y_size + src_nv12_uv_size;
> > 
> > I suspect there are support macros for calculating these somewhere.  Not a
> > big deal
> 
> Not change. (I don't find usable macros in libyuv so ...)

Ok, thanks.

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8390283 [details] [diff] [review]
B969395_webrtc_hw_codec.v04_01.patch

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

Definitely closer; what remains seems easy to resolve (modulo the question about encode/decode instances).

::: media/webrtc/signaling/signaling.gyp
@@ +223,5 @@
>              'SECLIB_OPENSSL',
>            ],
>  
>            'cflags_mozilla': [
> +            '-Wno-multichar',

Please add a comment why this is needed

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

Indent: either align with 'int' or indent 2 spaces.  Match the rest of the file is the primary target.

@@ +295,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
@@ +304,5 @@
> +  mPtrExtCodec = webrtc::ViEExternalCodec::GetInterface(mVideoEngine);
> +
> +  if (!mPtrExtCodec) {
> +    CSFLogError(logTag, "%s Unable to get external codec interface: %d ",
> +                __FUNCTION__,mPtrViEBase->LastError());

nit: space after commas

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +51,5 @@
> +    mLooper = new ALooper;
> +    mLooper->start();
> +    mMsg = new AMessage;
> +    mOmx = nullptr;
> +    isStarted = false;

Move most of these to the initializer list (except start())

@@ +140,5 @@
> +  mCallback(nullptr),
> +  mOmxEncoder(nullptr) {
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +
> +  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.

@@ +146,5 @@
> +
> +int32_t
> +WebrtcExtVideoEncoder::VerifyAndAllocate(const uint32_t minimumSize)
> +{
> +    if(minimumSize > mEncodedImage._size)

nit: space after "if".  Please apply across the file.

@@ +150,5 @@
> +    if(minimumSize > mEncodedImage._size)
> +    {
> +        // create buffer of sufficient size
> +        uint8_t* newBuffer = new uint8_t[minimumSize];
> +        if (newBuffer == NULL)

In mozilla code, 'new' cannot fail (and mozalloc() can't fail, though malloc() can).  remove OOM-handling code

@@ +157,5 @@
> +        }
> +        if(mEncodedImage._buffer)
> +        {
> +            // copy old data
> +            memcpy(newBuffer, mEncodedImage._buffer, mEncodedImage._size);

Do we ever actually need to copy the old encoded data?  I doubt it.

@@ +174,5 @@
> +  uint8_t* dst_uv = buffer + inputImage.stride(webrtc::kYPlane) *
> +                                inputImage.height();
> +  int dst_stride_uv = inputImage.stride(webrtc::kUPlane) * 2;
> +
> +  // Why NV12?  Because COLOR_FORMAT_YUV420_SEMIPLANAR.  Most hardwares are NV12-friendly.

nit: "Most hardware is"

@@ +333,5 @@
> +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> +  mEncodedImage._length = omxOut->size();
> +  uint8_t* data = omxOut->data();
> +
> +  if (VerifyAndAllocate(omxOut->size()) == -1) {

as mentioned above, I don't think VerifyAndAllocate() need to preserve the buffer contents

@@ +358,5 @@
> +  info.codecSpecific.VP8.keyIdx = -1;
> +
> +  // Generate a header describing a single fragment.
> +  webrtc::RTPFragmentationHeader header;
> +  memset(&header, 0, sizeof(header));

This is a class, you can't do this.

@@ +360,5 @@
> +  // Generate a header describing a single fragment.
> +  webrtc::RTPFragmentationHeader header;
> +  memset(&header, 0, sizeof(header));
> +  header.VerifyAndAllocateFragmentationHeader(1);
> +  header.fragmentationOffset[0] = 0;

VerifyAndAllocateFragmentationHeader guarantees new entries are 0

@@ +363,5 @@
> +  header.VerifyAndAllocateFragmentationHeader(1);
> +  header.fragmentationOffset[0] = 0;
> +  header.fragmentationLength[0] = mEncodedImage._length;
> +  header.fragmentationPlType[0] = 0;
> +  header.fragmentationTimeDiff[0] = 0;

Ditto

@@ +463,5 @@
> +  //        opaque/non-standard format.  It's not possible to negotiate the decoder
> +  //        to emit a specific colorspace
> +  //        For example, QCOM HW only support OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka
> +  //                     INTEL HW support OMX_COLOR_FormatYUV420PackedSemiPlanar
> +  size_t widthUV = width / 2;

Shouldn't this be (width+1)/2 like below?

@@ +478,5 @@
> +  int dst_i420_y_size = width * height;
> +  int dst_i420_uv_size = ((width + 1) / 2) * ((height + 1) / 2);
> +  int dst_i420_size = dst_i420_y_size + dst_i420_uv_size * 2;
> +  align_buffer_64(dst_i420_c, dst_i420_size);
> +  align_buffer_64(dst_i420_opt, dst_i420_size);

Do these align calls mean we have to make the allocated frame larger, depending on the geometry?

@@ +499,5 @@
> +    const sp<MediaCodec>& omx,
> +    const webrtc::EncodedImage& inputImage,
> +    int64_t renderTimeMs) {
> +
> +  static int64_t firstTime = -1;

This never has a value other than -1....

@@ +628,5 @@
> +
> +  sp<MediaCodec> omx = mOmxDecoder->mOmx;
> +  bool feedFrame = true;
> +
> +  status_t err = OK;

Remove initializer.  err is set within the while loop

@@ +631,5 @@
> +
> +  status_t err = OK;
> +
> +  while (feedFrame) {
> +    status_t err = FeedOMXInput(mOmxDecoder, omx, inputImage, renderTimeMs);

Alias!  remove status_t

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.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.

We may need to adjust this usage to have one or the other.

@@ +15,5 @@
> +
> +#ifndef WebrtcExtVideoCodec_h__
> +#define WebrtcExtVideoCodec_h__
> +
> +#include <queue>

Is this needed in the .h file, or only in a .cc file?  Is it needed at all?

Comment 40

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8390283 [details] [diff] [review]
B969395_webrtc_hw_codec.v04_01.patch

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

Adding back Chris to reviewers.

Comment 41

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #38)
> (In reply to qiang lu from comment #33)
> 
> > > ::: media/webrtc/signaling/signaling.gyp
> > > @@ +182,5 @@
> > > >          'HAVE_UINT8_T=1',
> > > >          'HAVE_UINT16_T=1',
> > > >          'HAVE_UINT32_T=1',
> > > >          'HAVE_UINT64_T=1',
> > > > +        'HAVE_SYS_UIO_H',
> > > 
> > > We shouldn't be taking that as a given, I think.  That should come from
> > > config
> > 
> > Could you please guide me how to move this Flag into config ?
> 
> Add it to a line like:
> MOZ_CHECK_HEADERS(sys/types.h netinet/in.h byteswap.h)
> 
> in the "Checks for header files" section of configure.in
> 
Done.

> 
> > > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > > @@ +68,5 @@
> > > >    delete mCurSendCodecConfig;
> > > >  
> > > > +  if (mPtrExtCodec) {
> > > > +    mPtrExtCodec->Release();
> > > > +    mPtrExtCodec = NULL;
> > > 
> > > remove nulling the pointer; none of the other Interface classes do it after
> > > Release().  Note that if bug 980304 lands first, just remove this entire
> > > block (it will handle Release())
> > 
> > Done. if B980304 landed, i will back here to remove those code.
> 
> That bug has landed, so you'll need to update this.
Done.

> 
> 
> > > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> > > @@ +283,5 @@
> > > >    webrtc::ViECodec* mPtrViECodec;
> > > >    webrtc::ViENetwork* mPtrViENetwork;
> > > >    webrtc::ViERender* mPtrViERender;
> > > >    webrtc::ViEExternalCapture*  mPtrExtCapture; // shared
> > > > +  webrtc::ViEExternalCodec*  mPtrExtCodec;
> > > 
> > > Note: if bug 980304 lands first, this will need to be changed to a
> > > ScopedCustomReleasePtr<> (and the .Release() removed)
> > Wait for B980304 lands
> 
> Ditto

Done.
> 
> 
> > > ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> > > @@ +62,5 @@
> > > > +    }
> > > > +    if (mLooper != nullptr) {
> > > > +      mLooper.clear();
> > > > +    }
> > > > +    if (mOmx != nullptr) {
> > > 
> > > for all of these, mozilla style is "if (ptr) { ... }"; don't compare to
> > > nullptr.
> > > 
> > > Please apply across the file/patch
> > I can't check mLooper/mOmx/mMsg whether is nullptr by if(ptr) since they are
> > sp<> point.
> > If i do check like normal mozilla style, below compile error were generated:
> > 
> >  0:14.76
> > /home/luq/Dev/firefox/firefox_for_android/x86_trunk/src/media/webrtc/
> > signaling/src/media-conduit/WebrtcExtVideoCodec.cpp:60:13: error: could not
> > convert '((mozilla::WebrtcOMX*)this)->mozilla::WebrtcOMX::mMsg' from
> > 'android::sp<android::AMessage>' to 'bool'
> 
> Ok.
> 
> 
> > > @@ +371,5 @@
> > > > +
> > > > +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> > > > +    webrtc::EncodedImageCallback* callback) {
> > > > +  CSFLogDebug(logTag, "%s ", __FUNCTION__);
> > > > +  mCallback = callback;
> > > 
> > > What thread does this happen on?  Does it happen on the thread where the
> > > encoder callback occur?  Can it happen on more than one thread?  Can it
> > > change during a stream of encode calls?  Is it only save to change between
> > > end of a previous callback and sending the next frame to be encoded?
> > > (assuming one frame "in-flight" at a time)
> > 
> > From my experience, it called by main webrtc thread which is thread where
> > encoder callback occur.
> 
> True; I suspect the webrtc encoder will call you back from one thread for a
> given instance of webrtc.  
> 
> Note that two instances of webrtc could result in two encoder threads (in
> different VideoEngine contexts).  For reference, if two separate threads try
> to use the OMX encoder, what happens?  Can the hardware support multiple
> encoder instances actively in use at the same time? (I presume one encode
> request would block if the HW was in use if this is supported.)  Or will
> only one of those get the hardware encoder, and the other get software OMX,
> or will one get HW and the other fail (and fall back to SW encoder in
> Webrtc)?
> 
> 
> > I don't think it can happen on more than one thread and couldn't be changed
> > during a stream of encode calls.
> 
> For a given stream, I believe this is correct (see above).
> 
> > I will keep those things your mentioned in head during the next work.
> 
AFAIK, our intel merrifield device can running two vp8 hw encode simultaneously. 
If we running more than 2 webrtc peer connection simulaneously, first 2 webrtc peer connections should work well using HW codec and others codec initilization will failed(couldn't initilize low level omx component).

And, webrtc is a resource-heavy usecase when running it on mobile device. i think the number of webrtcs can be run on fennec simulaneously should be limited.

I will file a bug to restrict the max number of webrtc can be run on fennec simulaneously.(may be dependency on device capability, cpu, mem, power etc.)
> 
> > > @@ +471,5 @@
> > > > +  uint8_t* src_nv12 = decoded->data();
> > > > +
> > > > +  int src_nv12_y_size = width * height;
> > > > +  int src_nv12_uv_size = ((width + 1) / 2) * ((height + 1) / 2) * 2;
> > > > +  int src_nv12_size = src_nv12_y_size + src_nv12_uv_size;
> > > 
> > > I suspect there are support macros for calculating these somewhere.  Not a
> > > big deal
> > 
> > Not change. (I don't find usable macros in libyuv so ...)
> 
> Ok, thanks.

Comment 42

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #39)
> Comment on attachment 8390283 [details] [diff] [review]
> B969395_webrtc_hw_codec.v04_01.patch
> 
> Review of attachment 8390283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Definitely closer; what remains seems easy to resolve (modulo the question
> about encode/decode instances).
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +223,5 @@
> >              'SECLIB_OPENSSL',
> >            ],
> >  
> >            'cflags_mozilla': [
> > +            '-Wno-multichar',
> 
> Please add a comment why this is needed
> 
Done.

> ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h
> @@ +287,5 @@
> > +     * @param encoder
> > +     * @result: on success, we will use the specified encoder
> > +     */
> > +     virtual MediaConduitErrorCode SetExternalSendCodec(int pltype,
> > +                   VideoEncoder* encoder) = 0;
> 
> Indent: either align with 'int' or indent 2 spaces.  Match the rest of the
> file is the primary target.
Done.

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

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +304,5 @@
> > +  mPtrExtCodec = webrtc::ViEExternalCodec::GetInterface(mVideoEngine);
> > +
> > +  if (!mPtrExtCodec) {
> > +    CSFLogError(logTag, "%s Unable to get external codec interface: %d ",
> > +                __FUNCTION__,mPtrViEBase->LastError());
> 
> nit: space after commas
Done.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +51,5 @@
> > +    mLooper = new ALooper;
> > +    mLooper->start();
> > +    mMsg = new AMessage;
> > +    mOmx = nullptr;
> > +    isStarted = false;
> 
> Move most of these to the initializer list (except start())
Done.

> 
> @@ +140,5 @@
> > +  mCallback(nullptr),
> > +  mOmxEncoder(nullptr) {
> > +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +
> > +  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.
Done.

> 
> @@ +146,5 @@
> > +
> > +int32_t
> > +WebrtcExtVideoEncoder::VerifyAndAllocate(const uint32_t minimumSize)
> > +{
> > +    if(minimumSize > mEncodedImage._size)
> 
> nit: space after "if".  Please apply across the file.
Done.

> 
> @@ +150,5 @@
> > +    if(minimumSize > mEncodedImage._size)
> > +    {
> > +        // create buffer of sufficient size
> > +        uint8_t* newBuffer = new uint8_t[minimumSize];
> > +        if (newBuffer == NULL)
> 
> In mozilla code, 'new' cannot fail (and mozalloc() can't fail, though
> malloc() can).  remove OOM-handling code
Done. remove all of those code

> 
> @@ +157,5 @@
> > +        }
> > +        if(mEncodedImage._buffer)
> > +        {
> > +            // copy old data
> > +            memcpy(newBuffer, mEncodedImage._buffer, mEncodedImage._size);
> 
> Do we ever actually need to copy the old encoded data?  I doubt it.
Done. remove all of those code

> 
> @@ +174,5 @@
> > +  uint8_t* dst_uv = buffer + inputImage.stride(webrtc::kYPlane) *
> > +                                inputImage.height();
> > +  int dst_stride_uv = inputImage.stride(webrtc::kUPlane) * 2;
> > +
> > +  // Why NV12?  Because COLOR_FORMAT_YUV420_SEMIPLANAR.  Most hardwares are NV12-friendly.
> 
> nit: "Most hardware is"
Done.

> 
> @@ +333,5 @@
> > +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> > +  mEncodedImage._length = omxOut->size();
> > +  uint8_t* data = omxOut->data();
> > +
> > +  if (VerifyAndAllocate(omxOut->size()) == -1) {
> 
> as mentioned above, I don't think VerifyAndAllocate() need to preserve the
> buffer contents
> 
> @@ +358,5 @@
> > +  info.codecSpecific.VP8.keyIdx = -1;
> > +
> > +  // Generate a header describing a single fragment.
> > +  webrtc::RTPFragmentationHeader header;
> > +  memset(&header, 0, sizeof(header));
> 
> This is a class, you can't do this.

Done. remove memset.
> 
> @@ +360,5 @@
> > +  // Generate a header describing a single fragment.
> > +  webrtc::RTPFragmentationHeader header;
> > +  memset(&header, 0, sizeof(header));
> > +  header.VerifyAndAllocateFragmentationHeader(1);
> > +  header.fragmentationOffset[0] = 0;
> 
> VerifyAndAllocateFragmentationHeader guarantees new entries are 0
Done.

> 
> @@ +363,5 @@
> > +  header.VerifyAndAllocateFragmentationHeader(1);
> > +  header.fragmentationOffset[0] = 0;
> > +  header.fragmentationLength[0] = mEncodedImage._length;
> > +  header.fragmentationPlType[0] = 0;
> > +  header.fragmentationTimeDiff[0] = 0;
> 
> Ditto
Done.

> 
> @@ +463,5 @@
> > +  //        opaque/non-standard format.  It's not possible to negotiate the decoder
> > +  //        to emit a specific colorspace
> > +  //        For example, QCOM HW only support OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka
> > +  //                     INTEL HW support OMX_COLOR_FormatYUV420PackedSemiPlanar
> > +  size_t widthUV = width / 2;
> 
> Shouldn't this be (width+1)/2 like below?
Not change. i do here by following the libyuv test case. i will double check whether it is ok in future test.

> 
> @@ +478,5 @@
> > +  int dst_i420_y_size = width * height;
> > +  int dst_i420_uv_size = ((width + 1) / 2) * ((height + 1) / 2);
> > +  int dst_i420_size = dst_i420_y_size + dst_i420_uv_size * 2;
> > +  align_buffer_64(dst_i420_c, dst_i420_size);
> > +  align_buffer_64(dst_i420_opt, dst_i420_size);
> 
> Do these align calls mean we have to make the allocated frame larger,
> depending on the geometry?
Not change. actually i just port it from libyuv and don't know whether align_buffer_64 is necessary here. i will try to know it.

> 
> @@ +499,5 @@
> > +    const sp<MediaCodec>& omx,
> > +    const webrtc::EncodedImage& inputImage,
> > +    int64_t renderTimeMs) {
> > +
> > +  static int64_t firstTime = -1;
> 
> This never has a value other than -1....
Done.

> 
> @@ +628,5 @@
> > +
> > +  sp<MediaCodec> omx = mOmxDecoder->mOmx;
> > +  bool feedFrame = true;
> > +
> > +  status_t err = OK;
> 
> Remove initializer.  err is set within the while loop
Done.

> 
> @@ +631,5 @@
> > +
> > +  status_t err = OK;
> > +
> > +  while (feedFrame) {
> > +    status_t err = FeedOMXInput(mOmxDecoder, omx, inputImage, renderTimeMs);
> 
> Alias!  remove status_t
Done.

> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.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.
> 
> We may need to adjust this usage to have one or the other.
Done. remove it which is copy from Webrtc project.

> 
> @@ +15,5 @@
> > +
> > +#ifndef WebrtcExtVideoCodec_h__
> > +#define WebrtcExtVideoCodec_h__
> > +
> > +#include <queue>
> 
> Is this needed in the .h file, or only in a .cc file?  Is it needed at all?
Done. remove it.

Comment 43

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v05.patch

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

> AFAIK, our intel merrifield device can running two vp8 hw encode
> simultaneously. 
> If we running more than 2 webrtc peer connection simulaneously, first 2
> webrtc peer connections should work well using HW codec and others codec
> initilization will failed(couldn't initilize low level omx component).
> 
> And, webrtc is a resource-heavy usecase when running it on mobile device. i
> think the number of webrtcs can be run on fennec simulaneously should be
> limited.
> 
> I will file a bug to restrict the max number of webrtc can be run on fennec
> simulaneously.(may be dependency on device capability, cpu, mem, power etc.)

Note: Some of the usecases for multiple encodes have the additional encodes be low-complexity (thumbnails,/simulcast, low frame rate (tab/screen-sharing), etc, so it's hard to reason ahead of time whether a specific number of peerconnections is reasonable.

If the initialization fails, it will fall back to software encoding (or should).  If an application overloads the device, we have code in the works to reduce resolution and framerate to compensate (we'll need to make that aware of the differences between software and hardware encode).

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8394021 [details] [diff] [review]
B969395_webrtc_hw_codec.v05.patch

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

r+ with various nits to resolve.  Upload a new patch and either set it to r+ if it lets you, or needinfo me to do so.  Note that for real use the SetRates() and one other function actually probably need to do something, and we need to support a fallback to software encoding (and ability to turn it off, at least for debugging/performance testing).

Thanks!

::: configure.in
@@ +2684,5 @@
> +*-android*|*-linuxandroid*)
> +# for stuff refer media/omx-plugin/include/kk/cutils/uio.h
> +  AC_DEFINE(HAVE_SYS_UIO_H)
> +  ;;
> +esac

just add  sys/uio.h to the MOZ_CHECK_HEADERS line above instead of this

::: media/webrtc/signaling/signaling.gyp
@@ +176,5 @@
>          '_NO_LOG4CXX',
>          'USE_SSLEAY',
>          '_CPR_USE_EXTERNAL_LOGGER',
>          'WEBRTC_RELATIVE_PATH',
> +        'HAVE_WEBRTC_VIDEO',

no certain what the difference is here...

@@ +227,5 @@
>              'GIPS_VER=3510',
>              'SECLIB_OPENSSL',
>            ],
>  
> +          # ignore "-Wmultichar" compile warning

I'm afraid this comment doesn't really help the reader understand why this was needed in the first place.  This is the equivalent to "x = x + 1; // add one to x".  We need something that helps someone coming to this understand why this was needed (instead of making source mods in the code, for example)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +290,3 @@
>  
>    webrtc::ViEExternalCapture* mPtrExtCapture; // shared
> +  

spurious space added

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h.rej
@@ +1,4 @@
> +--- VideoConduit.h
> ++++ VideoConduit.h
> +@@ -6,39 +6,57 @@
> + #define VIDEO_SESSION_H_

This file shouldn't be part of the patch

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +146,5 @@
> +  uint8_t* buffer = dstY;
> +  uint8_t* dst_y = buffer;
> +  int dst_stride_y = inputImage.stride(webrtc::kYPlane);
> +  uint8_t* dst_uv = buffer + inputImage.stride(webrtc::kYPlane) *
> +                                inputImage.height();

align with 'b' of 'buffer'

@@ +308,5 @@
> +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> +  mEncodedImage._length = omxOut->size();
> +  uint8_t* data = omxOut->data();
> +
> +  memcpy(mEncodedImage._buffer, data, mEncodedImage._length);

Add a comment here about our discussion about future work to avoid memcpy's (and perhaps file a followup bug and put the number here)

@@ +316,5 @@
> +
> +  mEncodedImage._completeFrame = true;
> +  mEncodedImage._frameType =
> +    outFlags & (MediaCodec::BUFFER_FLAG_SYNCFRAME | MediaCodec::BUFFER_FLAG_CODECCONFIG) ?
> +    webrtc::kKeyFrame:webrtc::kDeltaFrame;

Parenthesize the trinary in this form, since there's a bitwise op involved: 
(condition) ? a : b;
with parens around the condition, spaces around the :
If there's something using the trinary in larger expression, put parens around the entire trinary.

@@ +342,5 @@
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> +    webrtc::EncodedImageCallback* callback) {

don't wrap

@@ +374,5 @@
> +  return WEBRTC_VIDEO_CODEC_OK;
> +}
> +
> +// TODO
> +int32_t WebrtcExtVideoEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate) {

SetChannelParameters and SetRates (especially SetRates) will need to be supported for real use

@@ +434,5 @@
> +  //        opaque/non-standard format.  It's not possible to negotiate the decoder
> +  //        to emit a specific colorspace
> +  //        For example, QCOM HW only support OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka
> +  //                     INTEL HW support OMX_COLOR_FormatYUV420PackedSemiPlanar
> +  size_t widthUV = width / 2;

What if width is odd?  Should this be (width+1)/2?  (For the same reason the code below uses that)

@@ +449,5 @@
> +  int dst_i420_y_size = width * height;
> +  int dst_i420_uv_size = ((width + 1) / 2) * ((height + 1) / 2);
> +  int dst_i420_size = dst_i420_y_size + dst_i420_uv_size * 2;
> +  align_buffer_64(dst_i420_c, dst_i420_size);
> +  align_buffer_64(dst_i420_opt, dst_i420_size);

I checked, these allocate memory rounded up so you can use a 64-byte-aligned pointer (and keeps the original ptr around for free())

@@ +540,5 @@
> +  status_t err = omx->dequeueOutputBuffer(&index, &outOffset, &outSize, &outTime, &outFlags);
> +  if (err == INFO_FORMAT_CHANGED) {
> +    // handle format change
> +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +    goto end;

recode to nest instead of using goto end:
if (foo) {
...
} else if (bar) {
...
} else if (err != OK) {
...
} else {
  happy-case code
}
return err;

@@ +581,5 @@
> +    return WEBRTC_VIDEO_CODEC_ERROR;
> +  }
> +
> +  if (!mOmxDecoder->isStarted) {
> +

remove blank line

@@ +602,5 @@
> +  }
> +
> +  sp<MediaCodec> omx = mOmxDecoder->mOmx;
> +  bool feedFrame = true;
> +

remove blank line

Comment 46

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v06.patch

address jesup's latest comment.

Comment 47

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #45)
> Comment on attachment 8394021 [details] [diff] [review]
> B969395_webrtc_hw_codec.v05.patch
> 
> Review of attachment 8394021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with various nits to resolve.  Upload a new patch and either set it to r+
> if it lets you, or needinfo me to do so.  Note that for real use the
> SetRates() and one other function actually probably need to do something,
> and we need to support a fallback to software encoding (and ability to turn
> it off, at least for debugging/performance testing).
> 
> Thanks!
> 
> ::: configure.in
> @@ +2684,5 @@
> > +*-android*|*-linuxandroid*)
> > +# for stuff refer media/omx-plugin/include/kk/cutils/uio.h
> > +  AC_DEFINE(HAVE_SYS_UIO_H)
> > +  ;;
> > +esac
> 
> just add  sys/uio.h to the MOZ_CHECK_HEADERS line above instead of this
Done.

> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +176,5 @@
> >          '_NO_LOG4CXX',
> >          'USE_SSLEAY',
> >          '_CPR_USE_EXTERNAL_LOGGER',
> >          'WEBRTC_RELATIVE_PATH',
> > +        'HAVE_WEBRTC_VIDEO',
> 
> no certain what the difference is here...
actually, i subsitue two blank space for one tab in ahead of 'HAVE_WEBRTC_VIDEO'.

> 
> @@ +227,5 @@
> >              'GIPS_VER=3510',
> >              'SECLIB_OPENSSL',
> >            ],
> >  
> > +          # ignore "-Wmultichar" compile warning
> 
> I'm afraid this comment doesn't really help the reader understand why this
> was needed in the first place.  This is the equivalent to "x = x + 1; // add
> one to x".  We need something that helps someone coming to this understand
> why this was needed (instead of making source mods in the code, for example)
> 
Done.

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +290,3 @@
> >  
> >    webrtc::ViEExternalCapture* mPtrExtCapture; // shared
> > +  
> 
> spurious space added
> 
Done. remove it

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h.rej
> @@ +1,4 @@
> > +--- VideoConduit.h
> > ++++ VideoConduit.h
> > +@@ -6,39 +6,57 @@
> > + #define VIDEO_SESSION_H_
> 
> This file shouldn't be part of the patch

Done. remove this rej file from this patch.
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
> @@ +146,5 @@
> > +  uint8_t* buffer = dstY;
> > +  uint8_t* dst_y = buffer;
> > +  int dst_stride_y = inputImage.stride(webrtc::kYPlane);
> > +  uint8_t* dst_uv = buffer + inputImage.stride(webrtc::kYPlane) *
> > +                                inputImage.height();
> 
> align with 'b' of 'buffer'
Done.

> 
> @@ +308,5 @@
> > +  sp<ABuffer> omxOut = mOmxEncoder->getOutput()->itemAt(index);
> > +  mEncodedImage._length = omxOut->size();
> > +  uint8_t* data = omxOut->data();
> > +
> > +  memcpy(mEncodedImage._buffer, data, mEncodedImage._length);
> 
> Add a comment here about our discussion about future work to avoid memcpy's
> (and perhaps file a followup bug and put the number here)
> 
Done.

> @@ +316,5 @@
> > +
> > +  mEncodedImage._completeFrame = true;
> > +  mEncodedImage._frameType =
> > +    outFlags & (MediaCodec::BUFFER_FLAG_SYNCFRAME | MediaCodec::BUFFER_FLAG_CODECCONFIG) ?
> > +    webrtc::kKeyFrame:webrtc::kDeltaFrame;
> 
> Parenthesize the trinary in this form, since there's a bitwise op involved: 
> (condition) ? a : b;
> with parens around the condition, spaces around the :
> If there's something using the trinary in larger expression, put parens
> around the entire trinary.
> 
Done.

> @@ +342,5 @@
> > +  return WEBRTC_VIDEO_CODEC_OK;
> > +}
> > +
> > +int32_t WebrtcExtVideoEncoder::RegisterEncodeCompleteCallback(
> > +    webrtc::EncodedImageCallback* callback) {
> 
> don't wrap
> 
Done.

> @@ +374,5 @@
> > +  return WEBRTC_VIDEO_CODEC_OK;
> > +}
> > +
> > +// TODO
> > +int32_t WebrtcExtVideoEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate) {
> 
> SetChannelParameters and SetRates (especially SetRates) will need to be
> supported for real use
> 
I will file a bug to track the enable of those two critical apis after this preliminary patch be accepted.

> @@ +434,5 @@
> > +  //        opaque/non-standard format.  It's not possible to negotiate the decoder
> > +  //        to emit a specific colorspace
> > +  //        For example, QCOM HW only support OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka
> > +  //                     INTEL HW support OMX_COLOR_FormatYUV420PackedSemiPlanar
> > +  size_t widthUV = width / 2;
> 
> What if width is odd?  Should this be (width+1)/2?  (For the same reason the
> code below uses that)
Done. change to (width + 1) / 2;
> 
> @@ +449,5 @@
> > +  int dst_i420_y_size = width * height;
> > +  int dst_i420_uv_size = ((width + 1) / 2) * ((height + 1) / 2);
> > +  int dst_i420_size = dst_i420_y_size + dst_i420_uv_size * 2;
> > +  align_buffer_64(dst_i420_c, dst_i420_size);
> > +  align_buffer_64(dst_i420_opt, dst_i420_size);
> 
> I checked, these allocate memory rounded up so you can use a 64-byte-aligned
> pointer (and keeps the original ptr around for free())
> 
> @@ +540,5 @@
> > +  status_t err = omx->dequeueOutputBuffer(&index, &outOffset, &outSize, &outTime, &outFlags);
> > +  if (err == INFO_FORMAT_CHANGED) {
> > +    // handle format change
> > +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> > +    goto end;
> 
> recode to nest instead of using goto end:
> if (foo) {
> ...
> } else if (bar) {
> ...
> } else if (err != OK) {
> ...
> } else {
>   happy-case code
> }
> return err;
> 
Done.

> @@ +581,5 @@
> > +    return WEBRTC_VIDEO_CODEC_ERROR;
> > +  }
> > +
> > +  if (!mOmxDecoder->isStarted) {
> > +
> 
> remove blank line
Done.
> 
> @@ +602,5 @@
> > +  }
> > +
> > +  sp<MediaCodec> omx = mOmxDecoder->mOmx;
> > +  bool feedFrame = true;
> > +
> 
> remove blank line
Done.

Comment 48

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8390283 [details] [diff] [review]
B969395_webrtc_hw_codec.v04_01.patch

I think this has been obsoleted by v6.

Comment 49

[qiang lu]

Hi, 

Anything update?

Is there anything i can do for it?

Comment 50

[Randell Jesup [:jesup] (needinfo me)]

(In reply to qiang lu from comment #47)

> > @@ +374,5 @@
> > > +  return WEBRTC_VIDEO_CODEC_OK;
> > > +}
> > > +
> > > +// TODO
> > > +int32_t WebrtcExtVideoEncoder::SetRates(uint32_t newBitRate, uint32_t frameRate) {
> > 
> > SetChannelParameters and SetRates (especially SetRates) will need to be
> > supported for real use
> > 
> I will file a bug to track the enable of those two critical apis after this
> preliminary patch be accepted.

Note that we can't land a patch like this without ways to dynamically enable it's use (and likely disable it entirely), and it will need these APIs implements (SetRates() seems critical to real-world operation).  We could in theory land this preffed-off-by-default without SetRates/etc for testing.  If you want something to move this forward, I'd work on those two API calls.  :-)

Also, we're waiting on super-review right now.

Comment 51

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v08.patch

Minor updated for checking devices capability based on B973761.

And, enabling dynamic bitrate api again.

Comment 52

[qiang lu]

Comment on attachment 8401306 [details] [diff] [review]
B969395_webrtc_hw_codec.v08.patch

remove checkin Flags added by mistake.

Comment 53

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8401306 [details] [diff] [review]
B969395_webrtc_hw_codec.v08.patch

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +523,5 @@
> +                        video_codec.plName, video_codec.plType);
> +
> +#ifdef MOZILLA_INTERNAL_API
> +  CSFLogError(logTag, "%s Start to check hardware acceleration 1", __FUNCTION__);
> +  bool enabled = Preferences::GetBool("media.navigator.hardware.acceleration_enabled", false);

Didn't you define a differently named flag in bug 973761?

@@ +699,5 @@
> +    CSFLogDebug(logTag, "%s, Before ExtVideoCodec codec %s(%d)", __FUNCTION__,
> +                        video_codec.plName, video_codec.plType);
> +#ifdef MOZILLA_INTERNAL_API
> +    CSFLogError(logTag, "%s Start to check hardware acceleration 1", __FUNCTION__);
> +    bool enabled = Preferences::GetBool("media.navigator.hardware.acceleration_enabled", false);

Same remark.

Comment 54

[Gian-Carlo Pascutto [:gcp]]

Clearing checkin-need for 3 reasons: 

a) I have a concern the patch from the other bug isn't correctly taken into account, see above.
b) You'll have to set the review+ flag after you updated your patch and uploaded the new one (and after you got an r+ on the original, of course). The patch won't be checked in unless it was a positive review on it.
c) The first patch still has a review pending from cajbir, I sent him an email reminding him of that.

Comment 55

[qiang lu]

Attachment: B969395_webrtc_hw_codec.v09.patch

updated pref names according to the latest resolved patch of B973761.

Comment 56

[qiang lu]

Could you please take a look at the first patch? and guide me what else can i do for it?

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8401359 [details] [diff] [review]
B969395_webrtc_hw_codec.v09.patch

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

r+ - the only issues are naming issues and style nits, which don't require re-review.

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

File name WebrtcOMXVP8VideoCodec.* please

Ditto for the class names unless they're a class that's generic and not linked to VP8, in which case use OMX

@@ +169,5 @@
> +static void VideoCodecSettings2AMessage(
> +    sp<AMessage> &format,
> +    const webrtc::VideoCodec* codecSettings,
> +    const char* mime,
> +    bool isEncoder) {

Minor style nit:
mozilla bracing style is function-open-{ on new-line-at-left, not after )
I realize it's not 100% consistent, but especially for new code let's try to follow it.

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.h
@@ +14,5 @@
> +
> +namespace mozilla {
> +
> +struct EncodedFrame {
> +  uint32_t width_;

Mozilla style: mFoo, not Google style foo_

Comment 58

[u459114]

Comment on attachment 8401359 [details] [diff] [review]
B969395_webrtc_hw_codec.v09.patch

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

::: media/webrtc/signaling/src/media-conduit/WebrtcExtVideoCodec.cpp
@@ +601,5 @@
> +    err = FeedOMXInput(mOmxDecoder, omx, inputImage, renderTimeMs);
> +    feedFrame = (err == -EAGAIN);
> +    do {
> +      err = GetOMXOutput(mOmxDecoder, omx, inputImage, &mVideoFrame, mCallback, mFrameWidth, mFrameHeight);
> +    } while (err == INFO_OUTPUT_BUFFERS_CHANGED);

There is a big change in Bug 911046, please refer to this attachment:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=911046&attachment=8409510

To improve pipeline performance, John moved GetOMXOutput from Decoder thread into Drain thread. It does enhance pipeline throughput in H264 coding path. You may considerate have that design here as well

Comment 59

[u459114]

Hi Qiang,
Please make sure pass try server before label check-in-needed.

Here are instructions needed:
https://wiki.mozilla.org/Build:TryServer

Comment 60

[cajbir (:cajbir)]

Comment on attachment 8373230 [details] [diff] [review]
B969395_stub_library.v01.patch

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

::: media/omx-plugin/lib/kk/libstagefright_foundation/libstagefright_foundation.cpp
@@ +14,5 @@
> +#include "gui/Surface.h"
> +
> +
> +namespace android {
> +	

Remove whitespace here and everywhere else it is added in the file.

Comment 61

[qiang lu]

Attachment: merged patch for submitting to try-server

Comment 62

[qiang lu]

Building result from Try Server:
    https://hg.mozilla.org/try/rev/14976066b888

Comment 63

[qiang lu]

Attachment: B969395.patch

resolved conflict with mozilla-central and sending to Try Server.

building successful on Try Server

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b2588d41e3a

Comment 65

[Carsten Book [:Tomcat]]

sorry had to backout for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=40083901&tree=Mozilla-Inbound

Comment 66

[qiang lu]

I verified it using Try server with Android-x86 flag.

I think the error message in "https://tbpl.mozilla.org/php/getParsedLog.php?id=40083901&tree=Mozilla-Inbound" is because the build configure(mozconfig) is not set correctly.

Could you help to check the configuration of mozconfig? I had seen same error messages when build "firefox for android" with default desktop build configuration.

Comment 67

[John Lin [:jhlin][:jolin]]

(In reply to qiang lu from comment #66)
> I verified it using Try server with Android-x86 flag.
> 
> I think the error message in
> "https://tbpl.mozilla.org/php/getParsedLog.php?id=40083901&tree=Mozilla-
> Inbound" is because the build configure(mozconfig) is not set correctly.
> 
> Could you help to check the configuration of mozconfig? I had seen same
> error messages when build "firefox for android" with default desktop build
> configuration.

 Looks like it breaks all *non-Android* builds.
 I think you should modify moz.build and signaling.gyp and include your cpp files in source list only when 'OS==android' or something indicates it's a Fennec build (like what I did in attachment 8410064 [details] [diff] [review] for B2G).

Comment 68

[qiang lu]

Attachment: fixs cross-platform building issues

Fix cross-platform building issues according to John Lin's comment.

Comment 69

[qiang lu]

Building result from Try Server:
    https://tbpl.mozilla.org/?tree=Try&rev=f417962c27d4

Comment 70

[Carsten Book [:Tomcat]]

(In reply to qiang lu from comment #69)
> Building result from Try Server:
>     https://tbpl.mozilla.org/?tree=Try&rev=f417962c27d4

seems there are still issues on the android builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=40115445&tree=Try and failing tests or ?

Comment 71

[Carsten Book [:Tomcat]]

hm and one build error it seems https://tbpl.mozilla.org/php/getParsedLog.php?id=40105915&tree=Try

Comment 72

[qiang lu]

Hi John&Carsten,

Now, i am fixing the debug version building error mentioned by :Tomcat on Comment71.

But, For failing tests(see Comment 70), i don't know how to fix them. 

Could you please guard me how to handle those build failing for tests?  And how can i reproduce those build process on my local dev environment?(i mean where can i find mozconfig used by try server for those kinds of platforms) 

BTW, Whether all tests failing should be fixed before check-in?

BR,
Lu Qiang

Comment 73

[John Lin [:jhlin][:jolin]]

(In reply to qiang lu from comment #72)
> Hi John&Carsten,
> 
> Now, i am fixing the debug version building error mentioned by :Tomcat on
> Comment71.
> 
> But, For failing tests(see Comment 70), i don't know how to fix them. 
> 
> Could you please guard me how to handle those build failing for tests?  And
> how can i reproduce those build process on my local dev environment?(i mean
> where can i find mozconfig used by try server for those kinds of platforms) 
> 
> BTW, Whether all tests failing should be fixed before check-in?
> 
> BR,
> Lu Qiang

 MediaCodec is only available in Android 4.2+, right? Can you try restrict the build flag further so that ext codec won't be built on 2.x or 4.0? (In my case for B2G, ANDROID_VERSION is used)

Comment 74

[Carsten Book [:Tomcat]]

(In reply to qiang lu from comment #72)

Hi,

> Could you please guard me how to handle those build failing for tests?  And
> how can i reproduce those build process on my local dev environment?(i mean
> where can i find mozconfig used by try server for those kinds of platforms) 

i asked in irc and was told that the log (the full log in this case) might help its as example for the build failure https://tbpl.mozilla.org/php/getParsedLog.php?id=40105915&full=1&branch=try there is also the mozconfig included

> 
> BTW, Whether all tests failing should be fixed before check-in?
> 

It depend, in general for test failures caused by the checkedin patch, yes! otherwise a sheriff or someone else (like a developer around) might backout this change

If its a intermittent failures (like a failure totally unrelated to your checkin and not reproduceable on a second try) is ok and nothing you do about (beside also fixing that intermittent failure too if you are interested :)

Comment 75

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Carsten Book [:Tomcat] from comment #74)
> (In reply to qiang lu from comment #72)
> 
> Hi,
> 
> > Could you please guard me how to handle those build failing for tests?  And
> > how can i reproduce those build process on my local dev environment?(i mean
> > where can i find mozconfig used by try server for those kinds of platforms) 
> 
> i asked in irc and was told that the log (the full log in this case) might
> help its as example for the build failure
> https://tbpl.mozilla.org/php/getParsedLog.php?id=40105915&full=1&branch=try
> there is also the mozconfig included

That one is easy: there's a Debug-only log that references *timeUs - but there's no definition of timeUs anywhere in the file.  Cut-and-paste error, or a variable that got eliminated.

Generally, we probably don't #ifdef logging statements (though they may preprocess to "").

> 
> > 
> > BTW, Whether all tests failing should be fixed before check-in?
> > 
> 
> It depend, in general for test failures caused by the checkedin patch, yes!
> otherwise a sheriff or someone else (like a developer around) might backout
> this change

More like "will" back you out.  :-)

> If its a intermittent failures (like a failure totally unrelated to your
> checkin and not reproduceable on a second try) is ok and nothing you do
> about (beside also fixing that intermittent failure too if you are
> interested :)

tbpl tries to be smart and suggest possible intermittents; it's not already right though.  If you're unsure, ask someone on IRC about a specific orange.

Comment 76

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #75)
> (In reply to Carsten Book [:Tomcat] from comment #74)
> > (In reply to qiang lu from comment #72)
> > 
> > Hi,
> > 
> > > Could you please guard me how to handle those build failing for tests?  And
> > > how can i reproduce those build process on my local dev environment?(i mean
> > > where can i find mozconfig used by try server for those kinds of platforms) 
> > 
> > i asked in irc and was told that the log (the full log in this case) might
> > help its as example for the build failure
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=40105915&full=1&branch=try
> > there is also the mozconfig included
> 
> That one is easy: there's a Debug-only log that references *timeUs - but
> there's no definition of timeUs anywhere in the file.  Cut-and-paste error,
> or a variable that got eliminated.
> 
> Generally, we probably don't #ifdef logging statements (though they may
> preprocess to "").

Yes, i had addressed it by simple remove this debug log.

> > 
> > > 
> > > BTW, Whether all tests failing should be fixed before check-in?
> > > 
> > 
> > It depend, in general for test failures caused by the checkedin patch, yes!
> > otherwise a sheriff or someone else (like a developer around) might backout
> > this change
> 
> More like "will" back you out.  :-)
> 
> > If its a intermittent failures (like a failure totally unrelated to your
> > checkin and not reproduceable on a second try) is ok and nothing you do
> > about (beside also fixing that intermittent failure too if you are
> > interested :)
> 
> tbpl tries to be smart and suggest possible intermittents; it's not already
> right though.  If you're unsure, ask someone on IRC about a specific orange.

Actually, after some tests on try server, i found my patch always failed all of android related tests.

i am trying figure out which part in my patch cause those testing failure and find that may be libxul.mk is the root cause. see result show in 
https://tbpl.mozilla.org/?tree=Try&rev=91804f21d764 (without libxul.mk)
and
https://tbpl.mozilla.org/?tree=Try&rev=ae0ca680ba66 (with libxul.mk)

But, i still don't know how to fix this issue right now and will try to get help from IRC or mail-list if i couldn't get it done these days.

Comment 77

[Randell Jesup [:jesup] (needinfo me)]

(In reply to qiang lu from comment #76)
> Actually, after some tests on try server, i found my patch always failed all
> of android related tests.

Actually that appears to be failing Android 2.2 (Froyo) only; this is not a major problem.

> i am trying figure out which part in my patch cause those testing failure
> and find that may be libxul.mk is the root cause. see result show in 
> https://tbpl.mozilla.org/?tree=Try&rev=91804f21d764 (without libxul.mk)
> and
> https://tbpl.mozilla.org/?tree=Try&rev=ae0ca680ba66 (with libxul.mk)
> 
> But, i still don't know how to fix this issue right now and will try to get
> help from IRC or mail-list if i couldn't get it done these days.

Since it builds and runs fine without the libxul.mk change... I imagine it's not needed

Comment 78

[qiang lu]

(In reply to Randell Jesup [:jesup] from comment #77)
> (In reply to qiang lu from comment #76)
> > Actually, after some tests on try server, i found my patch always failed all
> > of android related tests.
> 
> Actually that appears to be failing Android 2.2 (Froyo) only; this is not a
> major problem.
> 
> > i am trying figure out which part in my patch cause those testing failure
> > and find that may be libxul.mk is the root cause. see result show in 
> > https://tbpl.mozilla.org/?tree=Try&rev=91804f21d764 (without libxul.mk)
> > and
> > https://tbpl.mozilla.org/?tree=Try&rev=ae0ca680ba66 (with libxul.mk)
> > 
> > But, i still don't know how to fix this issue right now and will try to get
> > help from IRC or mail-list if i couldn't get it done these days.
> 
> Since it builds and runs fine without the libxul.mk change... I imagine it's
> not needed

I need you help for resolving this issue.

Let me describe more details about this issue:

Basic information

1. /media/omx-plugin/lib/kk
	defined three stub libraries and used by 2, the real implementation of those librarie are located in real device.
	libstagefright.so
	libstagefright_foundation.so
	libutils.so	
	
2. /media/webrtc/signaling/sre/media-conduit/WebrtcExtVideoCodec.cc
	reference to the API declaration of 1's libraries for implementing HW acceleration in fennec webrtc utilize the Android MediaCodec native interface
	
3. /toolkit/library/libxul.mk
	generate libxul.so library which refer to WebrtcExtVideoCodec.o(generated by 2)
	--> I add 1's stub libraries dependency here otherwise build would failed when compile libxul.so
	
Problem
	all of android test casees failed to run with below error messages
		"WARNING -  PROCESS-CRASH | java-exception | java.lang.Exception: Error loading gecko libraries at org.mozilla.gecko.mozglue.GeckoLoader.loadGeckoLibsNative(Native Method)"
		 INFO -  06-18 20:15:46.164 E/GeckoLinker( 2256): /data/app/org.mozilla.fennec-1.apk!/assets/libxul.so: Relocation to NULL @0x01a0f798 for symbol "_ZN7android7RefBase11renameRefIdEPS0_PKvS3_"
			INFO -  06-18 20:15:46.164 E/GeckoLibLoad( 2256): Couldn't get a handle to libxul!
			INFO -  06-18 20:15:46.164 E/GeckoLibLoad( 2256): Throw
			INFO -  06-18 20:15:46.171 W/dalvikvm( 2256): threadid=14: thread exiting with uncaught exception (group=0x40b8c1f8)
			INFO -  06-18 20:15:46.171 E/GeckoAppShell( 2256): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 94 ("Gecko")
			INFO -  06-18 20:15:46.171 E/GeckoAppShell( 2256): java.lang.Exception: Error loading gecko libraries
			INFO -  06-18 20:15:46.171 E/GeckoAppShell( 2256): 	at org.mozilla.gecko.mozglue.GeckoLoader.loadGeckoLibsNative(Native Method)
			INFO -  06-18 20:15:46.171 E/GeckoAppShell( 2256): 	at org.mozilla.gecko.mozglue.GeckoLoader.loadGeckoLibs(GeckoLoader.java:289)
			INFO -  06-18 20:15:46.171 E/GeckoAppShell( 2256): 	at org.mozilla.gecko.GeckoThread.initGeckoEnvironment(GeckoThread.java:114)
			INFO -  06-18 20:15:46.171 E/GeckoAppShell( 2256): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:165)
	
	I think this issue is caused by libstagefright/libstagefright_foundation/libutils couldn't find in this try build device. Am i right? (i couldn't setup an local try server environment as Mozilla's TryServer for Android unittest)
	
How to fixs
	1. Find a way to eliminate the dependency of libxul.so.
		I couldn't find a simple way to do it. Could you please give me some hints?
	2. add libstagefright/libstagefright_foundation/libutils to the Android testcase runtime environment when execute those case on Try Server

Comment 79

[Gian-Carlo Pascutto [:gcp]]

>2. add libstagefright/libstagefright_foundation/libutils to the Android testcase runtime environment when execute those case on Try Server

That doesn't sound like a solution if it would stop Firefox from launching on a real 2.2 device! We can perhaps get someone with an Android 2.2 device to check if those libraries are present and what happens when we try to start Firefox with your patch on it.

We are busy deprecating Android 2.2, but that isn't finished yet: https://bugzilla.mozilla.org/show_bug.cgi?id=1017242

>Relocation to NULL @0x01a0f798 for symbol "_ZN7android7RefBase11renameRefIdEPS0_PKvS3_"

Is that warning specific to this problem? Are we perhaps using a C++/Android symbol that wasn't present on 2.2 yet?

Comment 80

[qiang lu]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #79)
> >2. add libstagefright/libstagefright_foundation/libutils to the Android testcase runtime environment when execute those case on Try Server
> 
> That doesn't sound like a solution if it would stop Firefox from launching
> on a real 2.2 device! We can perhaps get someone with an Android 2.2 device
> to check if those libraries are present and what happens when we try to
> start Firefox with your patch on it.
> 
> We are busy deprecating Android 2.2, but that isn't finished yet:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1017242
> 
> >Relocation to NULL @0x01a0f798 for symbol "_ZN7android7RefBase11renameRefIdEPS0_PKvS3_"
> 
> Is that warning specific to this problem? Are we perhaps using a C++/Android
> symbol that wasn't present on 2.2 yet?

Not only Android 2.2 has this problem, but also other Android 2.3, 4.0 has this same exception.

I couldn't know what symbol is relocated here clearly, but i seriously doubt that it is related to those functions i used in WebrtcExtVideoCodec.cpp which is exposed by libstagefright/libstagefright_foundation/libutils.

BTW, please refer to https://tbpl.mozilla.org/?tree=Try&rev=946b5ad9cd2b, which show all android testcase are failed to run due to same "unexpectation native library load error".

Comment 81

[John Lin [:jhlin][:jolin]]

(In reply to qiang lu from comment #80)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #79)
> > >2. add libstagefright/libstagefright_foundation/libutils to the Android testcase runtime environment when execute those case on Try Server
> > 
> > That doesn't sound like a solution if it would stop Firefox from launching
> > on a real 2.2 device! We can perhaps get someone with an Android 2.2 device
> > to check if those libraries are present and what happens when we try to
> > start Firefox with your patch on it.
> > 
> > We are busy deprecating Android 2.2, but that isn't finished yet:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1017242
> > 
> > >Relocation to NULL @0x01a0f798 for symbol "_ZN7android7RefBase11renameRefIdEPS0_PKvS3_"
> > 
> > Is that warning specific to this problem? Are we perhaps using a C++/Android
> > symbol that wasn't present on 2.2 yet?
> 
> Not only Android 2.2 has this problem, but also other Android 2.3, 4.0 has
> this same exception.

 I thought that MediaCodec is not there before Android 4.1/Jelly Bean so we can only support HW codec (for WebRTC and media recording) on JB+ platforms, which means HW codec related files should not be built for Android/Gonk before JB, right?
 Or am I missing something here?

> 
> I couldn't know what symbol is relocated here clearly, but i seriously doubt
> that it is related to those functions i used in WebrtcExtVideoCodec.cpp
> which is exposed by libstagefright/libstagefright_foundation/libutils.
> 
> BTW, please refer to https://tbpl.mozilla.org/?tree=Try&rev=946b5ad9cd2b,
> which show all android testcase are failed to run due to same "unexpectation
> native library load error".

Comment 82

[John Lin [:jhlin][:jolin]]

(Accidentally click the save button before finishing comment 81. Continue here. :$)

Perhaps those libraries you added in libxul.mk (media/omx-plugin/lib/kk/*) should be added only when ANDROID_VERSION >= 18?

Comment 83

[Gian-Carlo Pascutto [:gcp]]

>Perhaps those libraries you added in libxul.mk (media/omx-plugin/lib/kk/*) should be added only when ANDROID_VERSION >= 18?

I don't see how that will help given that ANDROID_VERSION at build time doesn't tell you anything about the device you end up running on. If this requires .so libs that aren't present on earlier Android versions, it will require dynamic opening at runtime, and you can't put a dependecy in libxul on them. (OpenSLES support already works like this)

Comment 84

[qiang lu]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #83)
> >Perhaps those libraries you added in libxul.mk (media/omx-plugin/lib/kk/*) should be added only when ANDROID_VERSION >= 18?
> 
> I don't see how that will help given that ANDROID_VERSION at build time
> doesn't tell you anything about the device you end up running on. If this
> requires .so libs that aren't present on earlier Android versions, it will
> require dynamic opening at runtime, and you can't put a dependecy in libxul
> on them. (OpenSLES support already works like this)

Ok, i will learn how OpenSLES is integration in fennec and try to fix this issue ASAP.

Comment 85

[qiang lu]

(In reply to qiang lu from comment #84)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #83)
> > >Perhaps those libraries you added in libxul.mk (media/omx-plugin/lib/kk/*) should be added only when ANDROID_VERSION >= 18?
> > 
> > I don't see how that will help given that ANDROID_VERSION at build time
> > doesn't tell you anything about the device you end up running on. If this
> > requires .so libs that aren't present on earlier Android versions, it will
> > require dynamic opening at runtime, and you can't put a dependecy in libxul
> > on them. (OpenSLES support already works like this)
> 
> Ok, i will learn how OpenSLES is integration in fennec and try to fix this
> issue ASAP.

After investigation, I think that it isn't possible do same thing as OpenSLES does for this patch.
    1. OpenSLES implemented by C, those dlopen/dlsym functionality are supported well in C world.
	2. MediaCodec(used in this patch) implemented by C++, there some problems in the support of "dlopen/dlsym" in C++.(http://tldp.org/HOWTO/C++-dlopen/)
	   for example, 
	       To use ABuffer/AMessage class, I need to create an instance of it, but that cannot be easily done from dlopen/dlsym since i couldn't change the
		   default implementation of them to expose create/destroy helper function for create/destroy a instance of ABuffer/AMessage.
		   
And also, I re-checked the implementation of fennec omx-plugin, but I found that it may have introduced name conflicts and other unforeseen problems.

So, could you please give me more hints about how to keep this patch going?

Comment 86

[Gian-Carlo Pascutto [:gcp]]

Edwin, do you know more about how me deal with depending on libstagefright etc on older Android devices?

Comment 87

[Gian-Carlo Pascutto [:gcp]]

FYI, we're currently exploring an alternate approach to omx-plugin: https://bugzilla.mozilla.org/show_bug.cgi?id=1014614. Rather than trying to use the Android decoders via the (unsupported) interface, it's using automatically-generated JNI bindings for the supported Java API.

Comment 88

[qiang lu]

Thanks for your information. 

That's exactly what i am thinking about to utilize the hardware accelertaion supported by android framework.

I will follow the status of https://bugzilla.mozilla.org/show_bug.cgi?id=1014614, and try to migrate present patch to the new approcah implemented in B1014614 once it's ready.

Comment 89

[Gian-Carlo Pascutto [:gcp]]

(In reply to qiang lu from comment #85)
 
> 	       To use ABuffer/AMessage class, I need to create an instance of it,
> but that cannot be easily done from dlopen/dlsym since i couldn't change the
> 		   default implementation of them to expose create/destroy helper function
> for create/destroy a instance of ABuffer/AMessage.

What about putting factory code for those classes in a separate library, hard linking that with the new Android libs you need, and then dlopen-ing your new library from the Firefox code? Would that circumvent this problem?

Comment 90

[qiang lu]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #89)
> 
> What about putting factory code for those classes in a separate library,
> hard linking that with the new Android libs you need, 
Could you please guide me how to hard linking "separate library" to "new Android libs"(libstagefright.so?)

> and then dlopen-ing your new library from the Firefox code? 
> Would that circumvent this problem?
I am afraid that exposing those mediacodec native api by dlopen-ing is not a easy work and be diffcult to maintain.

I think migrate this patch to follow what is implemented in B1014614(it seems that it will be completed in next week?) is more reasonable solution for introducing HW acceleration into Fennec.

Comment 91

[Gian-Carlo Pascutto [:gcp]]

(In reply to qiang lu from comment #90)

> Could you please guide me how to hard linking "separate library" to "new
> Android libs"(libstagefright.so?)

I mean linking with it at compile/link time rather than trying to use dlopen at runtime (i.e. same as the current patch does). This allows you to use the C++ parts you need for creating class instances and then expose an API to the Gecko part that's more amendable to dlopen.

> I am afraid that exposing those mediacodec native api by dlopen-ing is not a
> easy work and be diffcult to maintain.

I agree.
 
> I think migrate this patch to follow what is implemented in B(it
> seems that it will be completed in next week?) is more reasonable solution
> for introducing HW acceleration into Fennec.

Next week 2 weeks ago :-)

Comment 92

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #91)
>  
> > I think migrate this patch to follow what is implemented in B(it
> > seems that it will be completed in next week?) is more reasonable solution
> > for introducing HW acceleration into Fennec.
> 
> Next week 2 weeks ago :-)

Yeah, this isn't landing any time soon. YUV conversion issues alone will add some time.

Comment 93

[qiang lu]

since B1014614 had already marked fixed and seems had some test on decode side.

I am trying to re-implement this HW acceleration feature based on B1014614.

And, i will open two new Bugs for tracking the Encode and Decode side work separately ASAP.

Comment 94

[qiang lu]

Hi, James

Since B1014614 had landed at the end of October, I am back to work on B969395 where I want to follow what B101614 is done to access Android HW Video acceleration through standard MediaCodec API directly instead of using lower level undocumented native mediacodec api.

But, I had some problems when using generated MediaCodec.h .cpp in /media/webrtc/signaling. 
I include MediaCodec.h in WebrtcMediaCodecVP8VideoCodec.cpp under media/webrtc/signaling/src/media-conduit directory and use some API implemented in MediaCodec.cpp.
When I try to build it, compiler gives me some errors related to “internal string” roles breaks shown as bellows:
---------------------------------
     0:09.17 In file included from ../../../../dist/include/nsAString.h:11:0,
0:09.17                  from ../../../../dist/include/nsSubstring.h:10,
0:09.17                  from ../../../../dist/include/nsString.h:12,
0:09.17                  from ../../../../dist/include/nsGeoPosition.h:12,
0:09.17                  from ../../../../dist/include/AndroidJavaWrappers.h:13,
0:09.17                  from ../../../../dist/include/MediaCodec.h:9,
0:09.17                  from /home/luq/Dev/firefox/src/src/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
0:09.17 ../../../../dist/include/nsStringFwd.h:15:2: error: #error Internal string headers are not available from external-linkage code.
0:09.18 In file included from ../../../../dist/include/nsAString.h:23:0,
0:09.18                  from ../../../../dist/include/nsSubstring.h:10,
0:09.18                  from ../../../../dist/include/nsString.h:12,
0:09.18                  from ../../../../dist/include/nsGeoPosition.h:12,
0:09.18                  from ../../../../dist/include/AndroidJavaWrappers.h:13,
0:09.18                  from ../../../../dist/include/MediaCodec.h:9,
0:09.18                  from /home/luq/Dev/firefox/src/src/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
0:09.18 ../../../../dist/include/nsTSubstring.h:12:2: error: #error Cannot use internal string classes without MOZILLA_INTERNAL_API defined. Use the frozen header nsStringAPI.h instead.
0:09.18 In file included from ../../../../dist/include/nsAString.h:28:0,
0:09.18                  from ../../../../dist/include/nsSubstring.h:10,
0:09.18                  from ../../../../dist/include/nsString.h:12,
0:09.18                  from ../../../../dist/include/nsGeoPosition.h:12,
0:09.18                  from ../../../../dist/include/AndroidJavaWrappers.h:13,
0:09.18                  from ../../../../dist/include/MediaCodec.h:9,
0:09.18                  from /home/luq/Dev/firefox/src/src/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
0:09.18 ../../../../dist/include/nsTSubstring.h:12:2: error: #error Cannot use internal string classes without MOZILLA_INTERNAL_API defined. Use the frozen header nsStringAPI.h instead.
---------------------------------

And I try to resolve it by following 
    “XPCOM glue”, https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Glue
    “Migrating from Internal Linkage to Frozen Linkage”, https://developer.mozilla.org/en-US/docs/Migrating_from_Internal_Linkage_to_Frozen_Linkage
, but I couldn’t get any hints from those docs.

So, could you please guide me how can I use MediaCodec related API directly in my case?

Comment 95

[qiang lu]

Hi, gcp

Could you please also take a look at Comment 94 above?

Any suggestion is appreciated.

Comment 96

[qiang lu]

Hi, gcp

Could you please also take a look at Comment 94 above?

Any suggestion is appreciated.

Comment 97

[Gian-Carlo Pascutto [:gcp]]

Can you provide the patches and what you're trying to build exactly? This sounds like includes/header clashes.

Comment 98

[qiang lu]

Start a new bug (B1106958) to re-implement webrtc decoding using android.media.MediaCodec API instead low level native mediacodec api used here.

Comment 99

[James Willcox (:snorp) ([e-mail]) (he/him)]

I guess we need to use nsStringAPI.h instead of nsXPCOMStrings.h for this case. Try modifying the generated code to use nsStringAPI.h #ifndef MOZILLA_INTERNAL_API. You'll need to modify the code generator here: http://dxr.mozilla.org/mozilla-central/source/build/annotationProcessors/SDKProcessor.java#73

Comment 100

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 101

[Shmerl]

According to discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=1610199#c5
it was expected for libwebrtc to implement VAAPI support first, in order for Firefox to start using it.

However according to libwebrtc developers it's not going to happen, since VAAPI support for WebRTC in Chromium already exists and it lives outside of libwebrtc and it's not easy to move it inside the library:

See: https://bugs.chromium.org/p/webrtc/issues/detail?id=11296#c4

They therefore recommend Mozilla developers to follow the same approach and implement VAAPI support outside of libwebrtc.

Comment 102

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jib, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 103

[stalkerg]

Can we reuse WebRTC.rs https://github.com/webrtc-rs/webrtc instead of Chromium implementation?
The current situation is really sick and a long conference doesn't really work.

Regards,

Comment 104

[Paul Menzel]

Can we reuse WebRTC.rs https://github.com/webrtc-rs/webrtc instead of Chromium implementation?

Please create a separate bug report.

The current situation is really sick and a long conference doesn't really work.

For me it does work. What device, what Firefox version, what conference system? Please create a separate bug report.

Comment 105

[stalkerg]

Paul Menzel, why is this ticket still open if Firefox supports hardware acceleration for WebRTC on Linux (VAAPI)? As I understand, VAAPI supported for video decoding only, not for WebRTC case (google meet).

Regards,

Comment 106

[Paul Menzel]

This bug is a meta issue. Please create separate bug reports for issues to support the workflow of the developers, and save them time. Your report is missing a lot of details, please make sure to include them in your reports.

Comment 107

[stalkerg]

Sorry, but it's not a meta issue; this is a very specific ticket: "[WebRTC] Support hardware accelerated vp8 encoding & decoding for WebRTC" also "meta" not in keywords. Also, all reference tickets are solved and closed.
Maybe you are confused with https://bugzilla.mozilla.org/show_bug.cgi?id=1610199 ?

Comment 108

[Paul Menzel]

On GNU/Linux, VP8 decoding for WebRTC works for me in BigBlueButton. I think I had to set media.navigator.mediadatadecoder_vp8_hardware_enabled to true. Please create separate issues detailing what does not work for you. (I am out of this discussion.)