Blake Wu [:bwu][:blakewu] Reporter
	Description • 8 years ago

We should use platform HW codecs for encoding and decoding on webRTC to improve its performance.

	Randell Jesup [:jesup] (needinfo me)
	Comment 7 • 7 years ago

Forwarded reviews to those closer to this specific code

	Jean-Yves Avenard [:jya]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8845196 [details]
Bug 1265755 - separate JavaCallbacksSupport class declaration to a different header file;

https://reviewboard.mozilla.org/r/118160/#review120610

sorry, I don't feel comfortable reviewing this. I have no idea what there's to fixed, what it's fixing and the ocde it touches

	Gian-Carlo Pascutto [:gcp]
	Comment 9 • 7 years ago

Snorp, who should own/review this?

	Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018 Assignee
	Comment 10 • 7 years ago

Sorry for the confusion. These patches are still wip. I will ask John to help review after finishing the implementation.

	Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018 Assignee
	Comment 17 • 7 years ago

Almost finishing the implemenation except that I am still not able to request a key frame from android native encoder when there is packet loss.

	John Lin [:jhlin][:jolin]
	Comment 18 • 7 years ago

(In reply to Munro Mengjue Chiang [:mchiang] from comment #17)
> Almost finishing the implemenation except that I am still not able to
> request a key frame from android native encoder when there is packet loss.

 MediaCodec.setParameters [1] with PARAMETER_KEY_REQUEST_SYNC_FRAME (API 19+ only) may help here.

 [1] https://developer.android.com/reference/android/media/MediaCodec.html#setParameters(android.os.Bundle)

	Jean-Yves Avenard [:jya]
	Comment 19 • 7 years ago

Can we make sure whatever new interface we create, says a MediaDataEncoder, is almost identical to MediaDataDecoder ?

that is, using promises, and say a MediaDataEncoder::Encode() API in place of a Decode one.

For requesting a keyframe, I would assume that you would call Encode with the mKeyframe flag set to true; this would indicate a discontinuity and the requirement for a keyframe.

Thanks.

This will later be useful to integrate MediaDataDecoder / MediaDataEncoder with WebRTC

	Jean-Yves Avenard [:jya]
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8845197 [details]
Bug 1265755 - Support encoder case for CodecProxy;

https://reviewboard.mozilla.org/r/118162/#review127532

::: dom/media/platforms/android/RemoteDataDecoder.cpp:199
(Diff revision 5)
>  
>      mJavaCallbacks = CodecProxy::NativeCallbacks::New();
>      JavaCallbacksSupport::AttachNative(
>        mJavaCallbacks, mozilla::MakeUnique<CallbacksSupport>(this));
>  
> -    mJavaDecoder = CodecProxy::Create(mFormat,
> +    mJavaDecoder = CodecProxy::Create(false,

Please add inline comment indicating what that bolean is for.

Or use an enum with an explicit name

	Jean-Yves Avenard [:jya]
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8845198 [details]
Bug 1265755 - Implement remote vp8 encoder and enable/disable them with pref;

https://reviewboard.mozilla.org/r/118164/#review127536

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.h:88
(Diff revision 6)
> +
> +  virtual int32_t InitEncode(const webrtc::VideoCodec* codecSettings,
> +                             int32_t numberOfCores,
> +                             size_t maxPayloadSize) override;
> +
> +  virtual int32_t Encode(const webrtc::VideoFrame& inputImage,

virtual keyword is redundant and unecessary when the function is an override

	John Lin [:jhlin][:jolin]
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8845196 [details]
Bug 1265755 - separate JavaCallbacksSupport class declaration to a different header file;

https://reviewboard.mozilla.org/r/118160/#review127864

Looks good to me.

	John Lin [:jhlin][:jolin]
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8845197 [details]
Bug 1265755 - Support encoder case for CodecProxy;

https://reviewboard.mozilla.org/r/118162/#review127868

::: mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java:265
(Diff revision 6)
>          }
>          return true;
>      }
>  
>      @WrapForJNI
> +    public synchronized boolean setRates(int newBitRate) {

Reject when running on devices before API 19, or when this is a decoder.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:340
(Diff revision 6)
>  
>      @Override
> +    public final void setRates(int newBitRate) {
> +        Bundle params = new Bundle();
> +        params.putInt(MediaCodec.PARAMETER_KEY_VIDEO_BITRATE, newBitRate * 1000);
> +        mCodec.setParameters(params);

`MediaCodec.setParameters` is only availble in API 19+, please check it at runtime.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:352
(Diff revision 6)
>          mInputEnded = (flags & MediaCodec.BUFFER_FLAG_END_OF_STREAM) != 0;
>  
> +        if ((flags & MediaCodec.BUFFER_FLAG_KEY_FRAME) != 0) {
> +            Bundle params = new Bundle();
> +            params.putInt(MediaCodec.PARAMETER_KEY_REQUEST_SYNC_FRAME, 0);
> +            mCodec.setParameters(params);

ditto

	John Lin [:jhlin][:jolin]
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8845198 [details]
Bug 1265755 - Implement remote vp8 encoder and enable/disable them with pref;

https://reviewboard.mozilla.org/r/118164/#review127878

::: media/webrtc/signaling/src/media-conduit/MediaCodecVideoCodec.cpp:20
(Diff revision 7)
>  
>  WebrtcVideoEncoder* MediaCodecVideoCodec::CreateEncoder(CodecType aCodecType) {
>    CSFLogDebug(logTag,  "%s ", __FUNCTION__);
>    if (aCodecType == CODEC_VP8) {
> +    bool remoteEnabled = false;
> +    remoteEnabled = mozilla::Preferences::GetBool(

Nit: use `MediaPrefs` instead?

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:1013
(Diff revision 7)
> +    mJavaCallbacks = CodecProxy::NativeCallbacks::New();
> +
> +    JavaCallbacksSupport::AttachNative(
> +      mJavaCallbacks, mozilla::MakeUnique<CallbacksSupport>(mCallback));
> +
> +    if (inputImage.width() == 0 || inputImage.height() == 0) {

Nit: hoist to the beginning of method?

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:1029
(Diff revision 7)
> +    if (NS_FAILED(res)) {
> +      CSFLogDebug(logTag, "%s, CreateVideoFormat failed err = %d", __FUNCTION__, (int)res);
> +      return WEBRTC_VIDEO_CODEC_ERROR;
> +    }
> +
> +    res = format->SetInteger(nsCString("bitrate"), 300 * 1000);

Nit: use `MediaFormat::KEY_BIT_RATE` instead?

::: media/webrtc/signaling/src/media-conduit/WebrtcMediaCodecVP8VideoCodec.cpp:1103
(Diff revision 7)
> +    JavaCallbacksSupport::DisposeNative(mJavaCallbacks);
> +    mJavaCallbacks = nullptr;
> +  }
> +
> +  if (mConvertBuf) {
> +    delete mConvertBuf;

`delete[]`

	Pulsebot
	Comment 34 • 7 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7642856a6b3
separate JavaCallbacksSupport class declaration to a different header file; r=jolin
https://hg.mozilla.org/integration/autoland/rev/3f67d8a55f71
Support encoder case for CodecProxy; r=jolin
https://hg.mozilla.org/integration/autoland/rev/e00f4ffe0c56
Implement remote vp8 encoder and enable/disable them with pref; r=jolin

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a7642856a6b3
https://hg.mozilla.org/mozilla-central/rev/3f67d8a55f71
https://hg.mozilla.org/mozilla-central/rev/e00f4ffe0c56

	Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018 Assignee
	Comment 36 • 7 years ago

Please notice that in default we disable the hardware encoder.
Two pref need to be enabled to test the hardware encoders:

media.navigator.hardware.vp8_encode.acceleration_enabled
media.navigator.hardware.vp8_encode.acceleration_remote_enabled

	Blake Wu [:bwu][:blakewu] Reporter
	Comment 37 • 7 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]:
User can have better performance, like having higher frame rate or resolution, in using WebRTC. It can save more power consumption as well. 
[Affects Firefox for Android]:
Yes. 
[Suggested wording]:
Enable WebRTC to use hardware encoder. 
[Links (documentation, blog post, etc)]:
None.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 38 • 7 years ago

Added a note to Firefox 55 for developers about this pref, and that it will be turned on by default in 56.

https://developer.mozilla.org/en-US/Firefox/Releases/55#WebRTC

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 39 • 7 years ago

Adding a release note for 56 now that we seem to be turning the pref on by default.