[WebRTC] Use android.media.MediaCodec for decoding in WebRTC stack

RESOLVED FIXED in Firefox 40

Status

()

Core
WebRTC: Audio/Video
P3
normal
Rank:
33
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: qiang lu, Assigned: qiang lu)

Tracking

Trunk
mozilla40
x86_64
Android
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8531425 [details]
webrtc_mediacodec_hw_decode_v01.patch

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.71 Safari/537.36

Steps to reproduce:

[WIP]

Using official android.media.MediaCodec for decoding in WebRTC stack on latest Android

related Bugs
B969395, B1014614

BTW, since encoding related functions is not tested in B1014614, i try to implement the webrtc decoding parts in this patch firstly. once this patch is merged, i will submit another Bug to handle the webrtc encoding parts.
(Assignee)

Comment 1

3 years ago
I had some problems when using generated MediaCodec.h(which is under [output]/dist/include) in /media/webrtc/signaling. 

I include MediaCodec.h in WebrtcMediaCodecVP8VideoCodec.cpp and try to use some API implemented in MediaCodec.cpp following what is done in B1014614.

But 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/change the generated MediaCodec related API directly in my case?
or whether i need do some additional work(i am trying to understand how fennec interact with the android JAVA SDK ) to make the generated MediaCodec.h .cpp can be used by WebRTC stack?
Flags: needinfo?(gpascutto)
OS: Windows 8.1 → Android
(Assignee)

Comment 2

3 years ago
(In reply to qiang lu from comment #1)
> I had some problems when using generated MediaCodec.h(which is under
> [output]/dist/include) in /media/webrtc/signaling. 
> 
> I include MediaCodec.h in WebrtcMediaCodecVP8VideoCodec.cpp and try to use
> some API implemented in MediaCodec.cpp following what is done in B1014614.
> 
> But 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/change the generated MediaCodec
> related API directly in my case?
> or whether i need do some additional work(i am trying to understand how
> fennec interact with the android JAVA SDK ) to make the generated
> MediaCodec.h .cpp can be used by WebRTC stack?

And, alternately, whether i need to generate another similar MediaoCodec native codec(say WebrtcMediaCodec.h .cpp) according to what B1014614 is done and using those frozon headers instead internal headers?

Updated

3 years ago
Component: Audio/Video → WebRTC: Audio/Video
Product: Firefox for Android → Core
(Assignee)

Comment 3

3 years ago
Created attachment 8536549 [details] [diff] [review]
decoding_webrtc_vp8_by_android.media.MediaCodec

This is first patch for enabling vp8 hw codec on fennec using android.media.MediaCodec API

Add&change two files(WebrtcMediaCodec.h .cpp) generated from B1014614 for bypassing the issue when using them directly in webrtc stack.
Attachment #8531425 - Attachment is obsolete: true
Flags: needinfo?(gpascutto)
Attachment #8536549 - Flags: review?(gpascutto)
(Assignee)

Comment 4

3 years ago
Created attachment 8536558 [details] [diff] [review]
decoding_webrtc_vp8_by_android.media.MediaCodec

minor updates for adding preference check when enabling MediaCodec
Attachment #8536549 - Attachment is obsolete: true
Attachment #8536549 - Flags: review?(gpascutto)
(Assignee)

Updated

3 years ago
Attachment #8536558 - Flags: review?(gpascutto)
(Assignee)

Updated

3 years ago
Attachment #8536558 - Flags: review?(gpascutto) → review?(rjesup)

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

3 years ago
Hi, 

Happy holidays

And anything update about this patch?
Flags: needinfo?(rjesup)
(Assignee)

Comment 6

3 years ago
Created attachment 8545103 [details] [diff] [review]
using mediacodec for encoding&decoding in WebRTC stack

Added encode function in this patch
Attachment #8536558 - Attachment is obsolete: true
Attachment #8536558 - Flags: review?(rjesup)
Attachment #8545103 - Flags: review?(rjesup)
(Assignee)

Comment 7

3 years ago
Created attachment 8545253 [details] [diff] [review]
sing mediacodec for encoding&decoding in WebRTC stack

minor update for adding device capability check
Attachment #8545103 - Attachment is obsolete: true
Attachment #8545103 - Flags: review?(rjesup)
Attachment #8545253 - Flags: review?(rjesup)
(Assignee)

Comment 8

3 years ago
Could you help to find someone who can review this patch as soon as possible?
Flags: needinfo?(rjesup)
(Assignee)

Updated

3 years ago
Flags: needinfo?(gpascutto)
Flags: needinfo?(gpascutto)
Attachment #8545253 - Flags: review?(snorp)
Attachment #8545253 - Flags: feedback?(gpascutto)
Comment on attachment 8545253 [details] [diff] [review]
sing mediacodec for encoding&decoding in WebRTC stack

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

This is behind a pref, which seems alright, but do we want to use it on all intel hardware?

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp
@@ +188,5 @@
> +      }
> +
> +      if (encoder) {
> +        coder = WebrtcMediaCodec::CreateEncoderByType(nsCString(mime));
> +//        coder = WebrtcMediaCodec::CreateByCodecName(nsCString("OMX.Intel.VideoEncoder.VP8"));

Remove
Attachment #8545253 - Flags: review?(snorp) → review+
Comment on attachment 8545253 [details] [diff] [review]
sing mediacodec for encoding&decoding in WebRTC stack

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

Looks pretty good overall, I mostly have some questions to understand the code better and/or make sure some stuff that looks weird to me isn't small bugs.

I'm going to remove jesup from the reviewers because he seems overloaded anyway.

::: media/webrtc/moz.build
@@ +95,5 @@
>          GYP_DIRS['signalingtest'].input = 'signaling/signaling.gyp'
>          GYP_DIRS['signalingtest'].variables = gyp_vars.copy()
>          GYP_DIRS['signalingtest'].variables.update(
> +            build_for_test=1,
> +            moz_webrtc_mediacodec=0

Why is this needed? Do we need to fix something in the test?

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp
@@ +26,5 @@
> +
> +#include <vie_external_codec.h>
> +#include <webrtc/common_video/libyuv/include/webrtc_libyuv.h>
> +
> +#define WEBRTC_MEDIACODEC_DEBUG

This should probably only be there in debug mode.

@@ +51,5 @@
> +// - call QueueInput() to schedule a run to drain output. The input, aFrame,
> +//   should contains corresponding info such as image size and timestamps for
> +//   DrainOutput() implementation to construct data needed by encoded/decoded
> +//   callbacks.
> +// TODO: Bug 997110 - Revisit queue/drain logic. Current design assumes that

Note that this bug has been fixed for H264. If it's guaranteed the whitelisted VP8 encoders don't violate the invariant then I guess we're OK, but it's probably good to update the comments.

@@ +76,5 @@
> +      MonitorAutoUnlock unlock(mMonitor);
> +      NS_DispatchToMainThread(
> +        WrapRunnableNM<decltype(&ShutdownThread),
> +                       nsCOMPtr<nsIThread> >(&ShutdownThread, mThread));
> +      mThread = nullptr;

This looks like a bug: you're modifying mThread to null *outside* the lock, but there is other code calling things like mThread-> or generally assuming mThread != nullptr and assuming it's safe to do so while holding the lock.

@@ +260,5 @@
> +    // TODO: The hardware output buffer is likely to contain the pixels in a vendor-specific,
> +    //        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

This means that if we need to support QCOM hardware, we need to set a flag in the encoder whitelist that identifies the color format? Or is this detectable at runtime?

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

maybe nullptr for consistency

@@ +667,5 @@
> +//  err = omx->dequeueInputBuffer(&index, DEQUEUE_BUFFER_TIMEOUT_US);
> +//  if (err != OK) {
> +//    CSFLogError(logTag,  "%s WebrtcMediaCodecVP8VideoEncoder::Encode() dequeue OMX input buffer error:%d", __FUNCTION__, err);
> +//    return WEBRTC_VIDEO_CODEC_ERROR;
> +//  }

Needs to be removed or at the least have an explanation why it's commented out.

@@ +821,5 @@
> +
> +  // XXX
> +  // 1. implement MediaCodec's setParameters method
> +  // 2.find a way to initiate a Java Bundle instance as parameter for MediaCodec setParameters method.
> +  // mMediaCodecEncoder->setParameters

Does this mean the VP8 hardware encoder can't do bitrate adaptation?

It's generally a good idea to file follow up bugs for XXX's in Bugzilla.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h
@@ +41,5 @@
> +  virtual ~WebrtcMediaCodecVP8VideoEncoder() MOZ_OVERRIDE;
> +
> +  // Implement VideoEncoder interface.
> +  virtual const uint64_t PluginID() MOZ_OVERRIDE { return 0; }
> +  

nit: spurious whitespace

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +676,5 @@
> +      if (gfxInfo) {
> +        int32_t status;
> +        if (NS_SUCCEEDED(gfxInfo->GetFeatureStatus(nsIGfxInfo::FEATURE_WEBRTC_HW_ACCELERATION, &status))) {
> +          if (status != nsIGfxInfo::FEATURE_STATUS_OK) {
> +            NS_WARNING("XXX webrtc hardware acceleration blacklisted\n");

No need for the XXX here. Change the message to something like "VP8 encoder hardware is not whitelisted: disabling."

@@ +684,5 @@
> +              encoder = MediaCodecVideoCodec::CreateEncoder(MediaCodecVideoCodec::CodecType::CODEC_VP8);
> +              if (encoder) {
> +                return aConduit.SetExternalSendCodec(aConfig, encoder);
> +              } else {
> +                return kMediaConduitInvalidSendCodec;

Shouldn't this be kMediaConduitNoError to ensure proper fallback? (I didn't check the caller, just wondering because it's not the same codepath as not having the HW enabled)

::: widget/android/WebrtcAndroidJavaWrappers.cpp
@@ +24,5 @@
> +}
> +
> +WebrtcRefCountedJavaObject::~WebrtcRefCountedJavaObject() {
> +    if (mObject)
> +        jsjni_GetJNIForThread()->DeleteGlobalRef(mObject);

nit: braces

::: widget/android/WebrtcAndroidJavaWrappers.h
@@ +3,5 @@
> + * 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/. */
> +
> +#ifndef WebrtcAndroidJavaWrappers_h__
> +#define WebrtcAndroidJavaWrappers_h__

What's this doing that the existing classes don't?

Also purely FYI http://www.jnchen.com/blog/2015/01/new-smart-jni-reference-classes-for-fennec
Attachment #8545253 - Flags: review?(rjesup)
Attachment #8545253 - Flags: feedback?(gpascutto)
Attachment #8545253 - Flags: feedback-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9)

> This is behind a pref, which seems alright, but do we want to use it on all
> intel hardware?

The whitelist got added in the previous attempt that was using OMXCodec.
https://dxr.mozilla.org/mozilla-central/source/widget/android/GfxInfo.cpp#575

Updated

3 years ago
Flags: firefox-backlog+
Priority: -- → P3
(Assignee)

Comment 12

3 years ago
I refactored the code by following latest generated MediaCodec API and new smart jni reference.

But compiler gives me some errors related to “internal string” roles breaks shown as bellows again(in last patch, i forked the generated MediaCodec.h .cpp and changed some dependency files to bypass these errors, see comment1 & 3):

10:57.91 In file included from ../../../../dist/include/nsAString.h:11:0,
10:57.91                  from ../../../../dist/include/nsSubstring.h:10,
10:57.91                  from ../../../../dist/include/nsString.h:12,
10:57.91                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
10:57.91                  from ../../../../dist/include/MediaCodec.h:10,
10:57.91                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
10:57.91 ../../../../dist/include/nsStringFwd.h:15:2: error: #error Internal string headers are not available from external-linkage code.
10:57.92 In file included from ../../../../dist/include/nsAString.h:21:0,
10:57.92                  from ../../../../dist/include/nsSubstring.h:10,
10:57.92                  from ../../../../dist/include/nsString.h:12,
10:57.92                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
10:57.92                  from ../../../../dist/include/MediaCodec.h:10,
10:57.92                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
10:57.92 ../../../../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.
10:57.92 In file included from ../../../../dist/include/nsAString.h:26:0,
10:57.92                  from ../../../../dist/include/nsSubstring.h:10,
10:57.92                  from ../../../../dist/include/nsString.h:12,
10:57.92                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
10:57.92                  from ../../../../dist/include/MediaCodec.h:10,
10:57.92                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
10:57.92 ../../../../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.
10:57.96 In file included from ../../../../dist/include/nsStringGlue.h:21:0,
10:57.96                  from ../../../../dist/include/mozilla/BlockingResourceBase.h:25,
10:57.96                  from ../../../../dist/include/mozilla/Mutex.h:12,
10:57.96                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:8,
10:57.96                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:13:
10:57.96 Warning: enabled by default in /home/luq/Dev/firefox/latest_upstream/arm-objdir-droid/dist/include/nsStringAPI.h: "NS_MULTILINE_LITERAL_STRING" redefined
10:57.96 ../../../../dist/include/nsStringAPI.h:1260:0: warning: "NS_MULTILINE_LITERAL_STRING" redefined [enabled by default]
10:57.96 In file included from ../../../../dist/include/nsString.h:206:0,
10:57.96                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
10:57.96                  from ../../../../dist/include/MediaCodec.h:10,
10:57.96                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
10:57.96 ../../../../dist/include/nsLiteralString.h:25:0: note: this is the location of the previous definition
 
Could you please give me advice how can i resolve those String limitations this time ?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 13

3 years ago
Created attachment 8577691 [details] [diff] [review]
webrtc_mediacodec_hw_v1.2.patch

I refactored the patch using latest generated MediaCodec.h .cpp and new jni reference class.

But Compiler gives me some errors related to "internal string" roles breaks again:

0:14.08 In file included from ../../../../dist/include/nsAString.h:11:0,
 0:14.08                  from ../../../../dist/include/nsSubstring.h:10,
 0:14.08                  from ../../../../dist/include/nsString.h:12,
 0:14.08                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
 0:14.08                  from ../../../../dist/include/MediaCodec.h:10,
 0:14.08                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
 0:14.08 ../../../../dist/include/nsStringFwd.h:15:2: error: #error Internal string headers are not available from external-linkage code.
 0:14.08 In file included from ../../../../dist/include/nsAString.h:21:0,
 0:14.08                  from ../../../../dist/include/nsSubstring.h:10,
 0:14.08                  from ../../../../dist/include/nsString.h:12,
 0:14.08                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
 0:14.08                  from ../../../../dist/include/MediaCodec.h:10,
 0:14.08                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
 0:14.08 ../../../../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:14.08 In file included from ../../../../dist/include/nsAString.h:26:0,
 0:14.08                  from ../../../../dist/include/nsSubstring.h:10,
 0:14.08                  from ../../../../dist/include/nsString.h:12,
 0:14.08                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
 0:14.08                  from ../../../../dist/include/MediaCodec.h:10,
 0:14.08                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
 0:14.08 ../../../../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:14.12 In file included from ../../../../dist/include/nsStringGlue.h:21:0,
 0:14.12                  from ../../../../dist/include/nsThreadUtils.h:17,
 0:14.12                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:9,
 0:14.12                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:13:
 0:14.12 Warning: enabled by default in /home/luq/Dev/firefox/latest_upstream/arm-objdir-droid/dist/include/nsStringAPI.h: "NS_MULTILINE_LITERAL_STRING" redefined
 0:14.12 ../../../../dist/include/nsStringAPI.h:1260:0: warning: "NS_MULTILINE_LITERAL_STRING" redefined [enabled by default]
 0:14.12 In file included from ../../../../dist/include/nsString.h:206:0,
 0:14.12                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
 0:14.12                  from ../../../../dist/include/MediaCodec.h:10,
 0:14.12                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
 0:14.12 ../../../../dist/include/nsLiteralString.h:25:0: note: this is the location of the previous definition
 0:14.12 In file included from ../../../../dist/include/nsStringGlue.h:21:0,
 0:14.12                  from ../../../../dist/include/nsThreadUtils.h:17,
 0:14.12                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:9,
 0:14.12                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:13:
 0:14.12 Warning: enabled by default in /home/luq/Dev/firefox/latest_upstream/arm-objdir-droid/dist/include/nsStringAPI.h: "NS_MULTILINE_LITERAL_STRING_INIT" redefined
 0:14.12 ../../../../dist/include/nsStringAPI.h:1263:0: warning: "NS_MULTILINE_LITERAL_STRING_INIT" redefined [enabled by default]
 0:14.12 In file included from ../../../../dist/include/nsString.h:206:0,
 0:14.12                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
 0:14.12                  from ../../../../dist/include/MediaCodec.h:10,
 0:14.12                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
 0:14.12 ../../../../dist/include/nsLiteralString.h:26:0: note: this is the location of the previous definition
 0:14.13 In file included from ../../../../dist/include/nsStringGlue.h:21:0,
 0:14.13                  from ../../../../dist/include/nsThreadUtils.h:17,
 0:14.13                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:9,
 0:14.13                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:13:
 0:14.13 Warning: enabled by default in /home/luq/Dev/firefox/latest_upstream/arm-objdir-droid/dist/include/nsStringAPI.h: "NS_NAMED_MULTILINE_LITERAL_STRING" redefined
 0:14.13 ../../../../dist/include/nsStringAPI.h:1266:0: warning: "NS_NAMED_MULTILINE_LITERAL_STRING" redefined [enabled by default]
 0:14.13 In file included from ../../../../dist/include/nsString.h:206:0,
 0:14.13                  from ../../../../dist/include/mozilla/jni/Refs.h:10,
 0:14.13                  from ../../../../dist/include/MediaCodec.h:10,
 0:14.13                  from /home/luq/Dev/firefox/latest_upstream/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:12:
....

This time i can't find easy way to bypass those errors(in last patch, i tried to fix those issues by forking self WebrtcMediaCodec.h .cpp and its dependency files see comments 1 & 3).

Could you please give me advice how can i resolve those errors when using latest generated MediaCodec API with new jni reference class?
(Assignee)

Comment 14

3 years ago
Please ignore Comment12(its send by mistake)
Flags: needinfo?(gpascutto)
(Assignee)

Comment 15

3 years ago
please see Comment13 and webrtc_mediacodec_hw_v1.2.patch
Flags: needinfo?(gpascutto)
Rank: 33
>...and new smart jni reference.

0:14.08                  from ../../../../dist/include/mozilla/jni/Refs.h:10,

It looks like the new JNI stuff ends up using libxul's String APIs, but the WebRTC code tries to avoid depending on libxul, particularly for unit tests. You can define MOZILLA_INTERNAL_API for your code (but that might lead to some other build issues to fix up), I guess it makes no sense to try to link in hardware VP8 for tests anyway.
Flags: needinfo?(gpascutto)
(Assignee)

Comment 17

3 years ago
Created attachment 8587149 [details] [diff] [review]
webrtc_mediacodec_hw_v1.3.patch

After disabled VP8 HW acceleration for test, this patch can be build and package without any errors. And it can be run on Nexus 5 and our Moorefiled based device with HW acceleration support for both Enc and Dec.

For your comments on comments10:
Looks pretty good overall, I mostly have some questions to understand the code better and/or make sure some stuff that looks weird to me isn't small bugs.


::: media/webrtc/moz.build
@@ +95,5 @@
>          GYP_DIRS['signalingtest'].input = 'signaling/signaling.gyp'
>          GYP_DIRS['signalingtest'].variables = gyp_vars.copy()
>          GYP_DIRS['signalingtest'].variables.update(
> +            build_for_test=1,
> +            moz_webrtc_mediacodec=0


> Why is this needed? Do we need to fix something in the test?

we don't build VP8 hw for couldn't find a easy way to resolve build error in webrtc test.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp
@@ +26,5 @@
> +
> +#include <vie_external_codec.h>
> +#include <webrtc/common_video/libyuv/include/webrtc_libyuv.h>
> +
> +#define WEBRTC_MEDIACODEC_DEBUG

> This should probably only be there in debug mode.
Delete it.

@@ +51,5 @@
> +// - call QueueInput() to schedule a run to drain output. The input, aFrame,
> +//   should contains corresponding info such as image size and timestamps for
> +//   DrainOutput() implementation to construct data needed by encoded/decoded
> +//   callbacks.
> +// TODO: Bug 997110 - Revisit queue/drain logic. Current design assumes that


> Note that this bug has been fixed for H264. If it's guaranteed the whitelisted VP8 encoders don't violate the invariant then I guess we're OK, but it's probably good to update the comments.
Remove this comment.

@@ +76,5 @@
> +      MonitorAutoUnlock unlock(mMonitor);
> +      NS_DispatchToMainThread(
> +        WrapRunnableNM<decltype(&ShutdownThread),
> +                       nsCOMPtr<nsIThread> >(&ShutdownThread, mThread));
> +      mThread = nullptr;


> This looks like a bug: you're modifying mThread to null *outside* the lock, but there is other code calling things like mThread-> or generally assuming mThread != nullptr and assuming it's safe to do so while holding the lock.
I checked it, it seems it is not be called by any other threads in this patch.


@@ +260,5 @@
> +    // TODO: The hardware output buffer is likely to contain the pixels in a vendor-specific,
> +    //        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


> This means that if we need to support QCOM hardware, we need to set a flag in the encoder whitelist that identifies the color format? Or is this detectable at runtime?
This comment can be removed since it is ok for both Nexus 5 and Intel Moorifield based device using same format right now.

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


> maybe nullptr for consistency
changed it ot nuulptr.

@@ +667,5 @@
> +//  err = omx->dequeueInputBuffer(&index, DEQUEUE_BUFFER_TIMEOUT_US);
> +//  if (err != OK) {
> +//    CSFLogError(logTag,  "%s WebrtcMediaCodecVP8VideoEncoder::Encode() dequeue OMX input buffer error:%d", __FUNCTION__, err);
> +//    return WEBRTC_VIDEO_CODEC_ERROR;
> +//  }


> Needs to be removed or at the least have an explanation why it's commented out.
Delete.

@@ +821,5 @@
> +
> +  // XXX
> +  // 1. implement MediaCodec's setParameters method
> +  // 2.find a way to initiate a Java Bundle instance as parameter for MediaCodec setParameters method.
> +  // mMediaCodecEncoder->setParameters


> Does this mean the VP8 hardware encoder can't do bitrate adaptation?
> It's generally a good idea to file follow up bugs for XXX's in Bugzilla.
Now bitrate adpation is not support in this patch i will file a bug to track it soon.


::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h
@@ +41,5 @@
> +  virtual ~WebrtcMediaCodecVP8VideoEncoder() MOZ_OVERRIDE;
> +
> +  // Implement VideoEncoder interface.
> +  virtual const uint64_t PluginID() MOZ_OVERRIDE { return 0; }
> +  


> nit: spurious whitespace
Done.

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +676,5 @@
> +      if (gfxInfo) {
> +        int32_t status;
> +        if (NS_SUCCEEDED(gfxInfo->GetFeatureStatus(nsIGfxInfo::FEATURE_WEBRTC_HW_ACCELERATION, &status))) {
> +          if (status != nsIGfxInfo::FEATURE_STATUS_OK) {
> +            NS_WARNING("XXX webrtc hardware acceleration blacklisted\n");


> No need for the XXX here. Change the message to something like "VP8 encoder hardware is not whitelisted: disabling."
Done.


@@ +684,5 @@
> +              encoder = MediaCodecVideoCodec::CreateEncoder(MediaCodecVideoCodec::CodecType::CODEC_VP8);
> +              if (encoder) {
> +                return aConduit.SetExternalSendCodec(aConfig, encoder);
> +              } else {
> +                return kMediaConduitInvalidSendCodec;


> Shouldn't this be kMediaConduitNoError to ensure proper fallback? (I didn't check the caller, just wondering because it's not the same codepath as not having the HW enabled)

Done. using kMediaConduitNoError instead.

::: widget/android/WebrtcAndroidJavaWrappers.cpp
@@ +24,5 @@
> +}
> +
> +WebrtcRefCountedJavaObject::~WebrtcRefCountedJavaObject() {
> +    if (mObject)
> +        jsjni_GetJNIForThread()->DeleteGlobalRef(mObject);


> nit: braces
Done.


::: widget/android/WebrtcAndroidJavaWrappers.h
@@ +3,5 @@
> + * 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/. */
> +
> +#ifndef WebrtcAndroidJavaWrappers_h__
> +#define WebrtcAndroidJavaWrappers_h__

> What's this doing that the existing classes don't?
> Also purely FYI http://www.jnchen.com/blog/2015/01/new-smart-jni-reference-classes-for-fennec

refactoring this patch according to this new jni reference classes.
Attachment #8545253 - Attachment is obsolete: true
Attachment #8577691 - Attachment is obsolete: true
Attachment #8587149 - Flags: review?(gpascutto)
(Assignee)

Comment 18

3 years ago
Hi gcp,
   Could you please help to take a look at this patch if you have time?
Flags: needinfo?(gpascutto)
Comment on attachment 8587149 [details] [diff] [review]
webrtc_mediacodec_hw_v1.3.patch

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

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp
@@ +28,5 @@
> +
> +using namespace mozilla;
> +using namespace mozilla::widget::sdk;
> +
> +static const int32_t DECODER_TIMEOUT = 10000; // 10ms

10 * PR_USEC_PER_MSEC

@@ +84,5 @@
> +    mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +  }
> +
> +  void Stop() {
> +    MonitorAutoLock lock(mMonitor);

From your previous comment this isn't called by any other threads so you don't need this lock. Optionally add an assertion checking which thread is calling this (so it bombs out if something else does end up calling it).

Does the same apply to Start()?

Basically, either make the locking safe (see previous comment on mThread = nullptr below) or determine that it isn't needed and get rid of it.

@@ +185,5 @@
> +                     bool encoder) {
> +    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +    nsresult res = NS_OK;
> +
> +    if (mCoder == nullptr) {

nit: if (!mCoder)

@@ +389,5 @@
> +      }
> +
> +      if (mEnding) {
> +          ReleaseOutputBuffer(outputIndex, false);
> +          return NS_OK;

nit: inconsistent indentation

@@ +720,5 @@
> +    mEncodedImage._encodedHeight = inputImage.height();
> +    mEncodedImage._timeStamp = inputImage.timestamp();
> +    mEncodedImage.capture_time_ms_ = inputImage.timestamp();
> +
> +    //WebrtcBufferInfo bufferInfo;

Remove?

@@ +841,5 @@
> +
> +  // XXX
> +  // 1. implement MediaCodec's setParameters method
> +  // 2.find a way to initiate a Java Bundle instance as parameter for MediaCodec setParameters method.
> +  // mMediaCodecEncoder->setParameters

Indicate the follow up bug here when you've filed it.
Attachment #8587149 - Flags: review?(snorp)
Attachment #8587149 - Flags: review?(gpascutto)
Attachment #8587149 - Flags: review+
Comment on attachment 8587149 [details] [diff] [review]
webrtc_mediacodec_hw_v1.3.patch

ted? for the build/gyp parts
Flags: needinfo?(gpascutto)
Attachment #8587149 - Flags: review?(ted)
Comment on attachment 8587149 [details] [diff] [review]
webrtc_mediacodec_hw_v1.3.patch

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

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +791,5 @@
>                                            VideoCodecConfig* aConfig,
>                                            bool aIsSend)
>  {
>    if (aConfig->mName == "VP8" || aConfig->mName == "VP9") {
> +#ifdef MOZ_WEBRTC_MEDIACODEC

Is this supposed to activate for VP9?
The patch doesn't apply to current m-c. After doing the obvious fixups, I can't compile:

10:16.95 /home/morbo/hg/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:31:44: error: expected ';' at end of member declaration
10:16.95 /home/morbo/hg/mozilla-central/media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:31:46: error: 'MOZ_OVERRIDE' does not name a type
MOZ_OVERRIDE and MOZ_FINAL bit the dust a week or two ago; override and final now.
Comment on attachment 8587149 [details] [diff] [review]
webrtc_mediacodec_hw_v1.3.patch

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

Looks ok for now. I think eventually (maybe very soon), we're going to want to encode into a Surface so we can avoid the crazy YUV conversion mess the same we we do for decoding in AndroidDecoderModule. I think we'll also want to limit it to specific devices (maybe by requesting specific codec names as this patch did earlier).
Attachment #8587149 - Flags: review?(snorp) → review+
(Assignee)

Comment 25

3 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #21)
> Comment on attachment 8587149 [details] [diff] [review]
> webrtc_mediacodec_hw_v1.3.patch
> 
> Review of attachment 8587149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
> @@ +791,5 @@
> >                                            VideoCodecConfig* aConfig,
> >                                            bool aIsSend)
> >  {
> >    if (aConfig->mName == "VP8" || aConfig->mName == "VP9") {
> > +#ifdef MOZ_WEBRTC_MEDIACODEC
> 
> Is this supposed to activate for VP9?

It didn't support VP9 by now, i will updated those to support VP8 only and address those comments on Comments19.
(Assignee)

Comment 26

3 years ago
Hi ted,
Could you please have a look at this patch?

If everything ok ,i will get it to test on try-server ASAP.

luqiang
Flags: needinfo?(ted)
(Assignee)

Comment 27

3 years ago
Hi, gcp

I can't ping to ted...

Could you please tell me whether i can find a super-reviewer to get this patch going?
Flags: needinfo?(gpascutto)
Comment on attachment 8587149 [details] [diff] [review]
webrtc_mediacodec_hw_v1.3.patch

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

::: build/gyp.mozbuild
@@ +81,5 @@
>      else:
>          gyp_vars.update(
> +          moz_webrtc_mediacodec=1
> +        )
> +        gyp_vars.update(

Not sure why this is adding a second call to gyp_vars.update, just stick this line into the existing one.
Attachment #8587149 - Flags: review?(ted) → review+
Flags: needinfo?(ted)
I nagged Ted on IRC to have a look. r+ everywhere, so time to rebase to mozilla-central and see how it does on try!
Flags: needinfo?(gpascutto)
(Assignee)

Comment 30

3 years ago
Created attachment 8597092 [details] [diff] [review]
webrtc_mediacodec_hw_v1.4.patch

minors changes for addressing some nits comments.
And i put it to Try-Server and seems it works well on Try Server.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f3b3a3a22a0
Attachment #8587149 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8597092 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea790a14bc11
Assignee: nobody → qiang.lu
Keywords: checkin-needed
Comment on attachment 8597092 [details] [diff] [review]
webrtc_mediacodec_hw_v1.4.patch

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

I realize this has already landed, but I have some followup-nits and bugs that need to be dealt with.

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp
@@ +23,5 @@
> +#include "libyuv/convert_from.h"
> +#include "libyuv/convert.h"
> +#include "libyuv/row.h"
> +
> +#include <webrtc/common_video/libyuv/include/webrtc_libyuv.h>

This should have been "'s, not <'s

@@ +209,5 @@
> +          CSFLogDebug(logTag, "WebrtcAndroidMediaCodec::%s, CreateEncoderByType failed err = %d", __FUNCTION__, res);
> +          return NS_ERROR_FAILURE;
> +        }
> +
> +        res = format->SetInteger(nsCString("bitrate"), 1000*300);

This should be coming from the pref, but likely anything set here for it will get overridden, so it's likely fine.

@@ +210,5 @@
> +          return NS_ERROR_FAILURE;
> +        }
> +
> +        res = format->SetInteger(nsCString("bitrate"), 1000*300);
> +        res = format->SetInteger(nsCString("bitrate-mode"), 2);

Comment on what this means please

@@ +211,5 @@
> +        }
> +
> +        res = format->SetInteger(nsCString("bitrate"), 1000*300);
> +        res = format->SetInteger(nsCString("bitrate-mode"), 2);
> +        res = format->SetInteger(nsCString("color-format"), 21);

Ditto

@@ +213,5 @@
> +        res = format->SetInteger(nsCString("bitrate"), 1000*300);
> +        res = format->SetInteger(nsCString("bitrate-mode"), 2);
> +        res = format->SetInteger(nsCString("color-format"), 21);
> +        res = format->SetInteger(nsCString("frame-rate"), 30);
> +        res = format->SetInteger(nsCString("i-frame-interval"), 100);

We should never be sending periodic iframes

@@ +240,5 @@
> +    }
> +
> +    mEnding = false;
> +
> +    nsresult res;

this should be 'rv' for style consistency

@@ +267,5 @@
> +    return NS_OK;
> +  }
> +
> +  void GenerateVideoFrame(
> +      size_t width, size_t height, uint32_t timeStamp,

width and height shouldn't be size_t.  uint16_t or uint32_t.

@@ +280,5 @@
> +      return;
> +    }
> +
> +    uint8_t* src_nv12 = static_cast<uint8_t *>(decoded);
> +    int src_nv12_y_size = width * height;

not really int; this should be size_t

@@ +286,5 @@
> +    uint8_t* dstY = videoFrame->buffer(webrtc::kYPlane);
> +    uint8_t* dstU = videoFrame->buffer(webrtc::kUPlane);
> +    uint8_t* dstV = videoFrame->buffer(webrtc::kVPlane);
> +
> +    libyuv::NV12ToI420(src_nv12, width,

I presume the calculates the correct strides/etc for NV12

@@ +291,5 @@
> +                       src_nv12 + src_nv12_y_size, (width + 1) & ~1,
> +                       dstY, width,
> +                       dstU, (width + 1) / 2,
> +                       dstV,
> +                       (width + 1) / 2,

should be on previous line to be consistent (nit)

@@ +401,5 @@
> +      JNIEnv* env = jsjni_GetJNIForThread();
> +      jobject buffer = env->GetObjectArrayElement(mOutputBuffers, outputIndex);
> +      if (buffer) {
> +        // The buffer will be null on Android L if we are decoding to a Surface
> +        void* directBuffer = env->GetDirectBufferAddress(buffer);

I presume this code (and following) is ok on Android L with a null buffer

@@ +623,5 @@
> +    const webrtc::VideoCodec* codecSettings,
> +    int32_t numberOfCores,
> +    uint32_t maxPayloadSize) {
> +  mMaxPayloadSize = maxPayloadSize;
> +  CSFLogDebug(logTag,  "%s, w = %d, h = %d", __FUNCTION__, codecSettings->width, codecSettings->height);

%u would be more correct, but since they're unsigned short it's not going to get a warning

@@ +639,5 @@
> +    mMediaCodecEncoder = new WebrtcAndroidMediaCodec();
> +  }
> +
> +  if (!mMediaCodecEncoder->isStarted) {
> +    if (inputImage.width() == 0 || inputImage.height() == 0) {

Since they're ints, <= 0 would be better

@@ +761,5 @@
> +        int32_t flags;
> +        bufferInfo->Flags(&flags);
> +
> +        // The buffer will be null on Android L if we are decoding to a Surface
> +        void* directBuffer = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buffer)) + offset;

same comment

@@ +849,5 @@
> +    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
> +  }
> +
> +  // XXX
> +  // 1. implement MediaCodec's setParameters method

This codec implementation cannot be enabled-by-default without support for setting the rates.  Please file a followup bug for this (per previous request from gcp) and include in your followup patch a pointer to the bug here

@@ +869,5 @@
> +bool WebrtcMediaCodecVP8VideoDecoder::ResetInputBuffers() {
> +  mInputBuffers = mMediaCodecDecoder->GetInputBuffers();
> +
> +  if (!mInputBuffers)
> +    return false;

braces, or "return mInputBuffers;" (or static_cast<bool>)

@@ +878,5 @@
> +bool WebrtcMediaCodecVP8VideoDecoder::ResetOutputBuffers() {
> +  mOutputBuffers = mMediaCodecDecoder->GetOutputBuffers();
> +
> +  if (!mOutputBuffers)
> +    return false;

ditto

@@ +933,5 @@
> +
> +    res = mMediaCodecDecoder->Start();
> +
> +    if (NS_FAILED(res)) {
> +      mMediaCodecDecoder->isStarted = false;

delete this line; we're inside if (!isStarted)

@@ +997,5 @@
> +}
> +
> +int32_t WebrtcMediaCodecVP8VideoDecoder::Reset() {
> +  CSFLogDebug(logTag,  "%s ", __FUNCTION__);
> +  return WEBRTC_VIDEO_CODEC_OK;

Either this should reset, or add a comment why it's ok to ignore it

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h
@@ +70,5 @@
> +class WebrtcMediaCodecVP8VideoDecoder : public WebrtcVideoDecoder {
> +public:
> +  WebrtcMediaCodecVP8VideoDecoder();
> +
> +  virtual ~WebrtcMediaCodecVP8VideoDecoder() override;

is override really needed here?

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +859,5 @@
> +  if (aConfig->mName == "VP8") {
> +#ifdef MOZ_WEBRTC_MEDIACODEC
> +     if (aIsSend) {
> +#ifdef MOZILLA_INTERNAL_API
> +       bool enabled = mozilla::Preferences::GetBool("media.navigator.hardware.vp8_encode.acceleration_enabled", false);

This should include a patch to all.js to add this pref, unless it's purposely hidden (which perhaps it is for now).  If it is purposely hidden (such as due to the lack of ability to change rates), make sure a followup bug to enable it is filed.

@@ +884,5 @@
> +         }
> +       }
> +     } else {
> +#ifdef MOZILLA_INTERNAL_API
> +       bool enabled = mozilla::Preferences::GetBool("media.navigator.hardware.vp8_decode.acceleration_enabled", false);

ditto
(Assignee)

Comment 33

3 years ago
Got it, i will address those comments(reopen it) as soon as this patch landed in comm-central.

BTW, Bug1158449 is filed for tracing the implementation of SetRates status.
(In reply to qiang lu from comment #33)
> Got it, i will address those comments(reopen it) as soon as this patch
> landed in comm-central.
> 
> BTW, Bug1158449 is filed for tracing the implementation of SetRates status.

Ok, thanks - please just file another followup bug rather than reopening this one
(Assignee)

Updated

3 years ago
Flags: needinfo?(wkocher)
(Assignee)

Comment 35

3 years ago
Hi KWierso,

Could you please have a look at this bug, whether it is ready to merged into mozilla-central?
Flags: needinfo?(wkocher)
(Assignee)

Comment 36

3 years ago
Hi KWierso,

Could you please have a look at this bug, whether it is ready to merged into mozilla-central?
Flags: needinfo?(wkocher)
Carsten (Tomcat on IRC) or Ryan (RyanVM) will probably get to this far sooner than I will, as it's 10PM on a Sunday night for me, but Europe and US's east coast should be waking up soon. :)
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/ea790a14bc11
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
This change causes failure to compile on Android - I have logged a new issue for that at https://bugzilla.mozilla.org/show_bug.cgi?id=1158931

Updated

3 years ago
Depends on: 1158931
You need to log in before you can comment on or make changes to this bug.