Closed Bug 1265755 Opened 9 years ago Closed 8 years ago

[Firefox for Android] Utilize hardware encoders to save power consumption & improve performance (behind pref)

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Platform:
Unspecified
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
relnote-firefox --- 56+
firefox55 --- fixed

People

(Reporter: bwu, Assigned: mchiang)

References

Details

Attachments

(3 files)

Bug 1265755 - separate JavaCallbacksSupport class declaration to a different header file;
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1265755 - Support encoder case for CodecProxy;
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1265755 - Implement remote vp8 encoder and enable/disable them with pref;
59 bytes, text/x-review-board-request
jhlin
: review+
Details
We should use platform HW codecs for encoding and decoding on webRTC to improve its performance.
Depends on: 1257777
Component: Audio/Video → WebRTC: Audio/Video
Product: Firefox for Android → Core
Summary: Get HW (platform) encoders and decoders used on WebRTC → [Firefox for Android] Utilize hardware encoders & decoders to save power consumption & improve performance
Rank: 25
Priority: -- → P2
Summary: [Firefox for Android] Utilize hardware encoders & decoders to save power consumption & improve performance → [Firefox for Android] Utilize hardware encoders to save power consumption & improve performance
OS: Unspecified → Android
Assignee: nobody → mchiang
Attachment #8845197 - Flags: review?(rjesup) → review?(gpascutto)
Attachment #8845196 - Flags: review?(rjesup) → review?(jyavenard)
Forwarded reviews to those closer to this specific code
Attachment #8845196 - Flags: review?(jyavenard)
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
Snorp, who should own/review this?
Flags: needinfo?(snorp)
Flags: needinfo?(snorp)
Attachment #8845196 - Flags: review?(jolin)
Attachment #8845197 - Flags: review?(gpascutto) → review?(jolin)
Sorry for the confusion. These patches are still wip. I will ask John to help review after finishing the implementation.
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.
(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)
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
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
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
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.
Attachment #8845196 - Flags: review?(jolin) → 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
Attachment #8845197 - Flags: review?(jolin) → 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[]`
Attachment #8845198 - Flags: review?(jolin) → review+
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
Keywords: checkin-needed
QA Contact: bogdan.surd
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
Depends on: 1379926
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.
relnote-firefox: --- → ?
Summary: [Firefox for Android] Utilize hardware encoders to save power consumption & improve performance → [Firefox for Android] Utilize hardware encoders to save power consumption & improve performance (behind pref)
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
Adding a release note for 56 now that we seem to be turning the pref on by default.
relnote-firefox: ? → 56+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: