Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(34)

Issue 2067103002: Avoid unnecessary HW video encoder reconfiguration (Closed)

Created:
4 years, 6 months ago by skvlad
Modified:
4 years, 6 months ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, pbos-webrtc, perkj_webrtc, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Avoid unnecessary HW video encoder reconfiguration This change reduces the number of times the Android hardware video encoder is reconfigured when making an outgoing call. With this change, the encoder should only be initialized once as opposed to the ~3 times it happens currently. Before the fix, the following sequence of events caused the extra reconfigurations: 1. After the SetLocalDescription call, the WebRtcVideoSendStream is created. All frames from the camera are dropped until the corresponding VideoSendStream is created. 2. SetRemoteDescription() triggers the VideoSendStream creation. At this point, the encoder is configured for the first time, with the frame dimensions set to a low resolution default (176x144). 3. When the first video frame is received from the camera after the VideoSendStreamIsCreated, the encoder is reconfigured to the correct dimensions. If we are using the Android hardware encoder, the default configuration is set to encode from a memory buffer (use_surface=false). 4. When the frame is passed down to the encoder in androidmediaencoder_jni.cc EncodeOnCodecThread(), it may be stored in a texture instead of a memory buffer. In this case, yet another reconfiguration takes place to enable encoding from a texture. 5. Even if the resolution and texture flag were known at the start of the call, there would be a reconfiguration involved if the camera is rotated (such as when making a call from a phone in portrait orientation). The reason for that is that at construction time, WebRtcVideoEngine2 sets the VideoSinkWants structure parameter to request frames rotated by the source; the early frames will then arrive in portrait resolution. When the remote description is finally set, if the rotation RTP extension is supported by the remote receiver, the source is asked to provide non-rotated frames. The very next frame will then arrive in landscape resolution with a non-zero rotation value to be applied by the receiver. Since the encoder was configured with the last (portrait) frame size, it's going to need to be reconfigured again. The fix makes the following changes: 1. WebRtcVideoSendStream::OnFrame() now caches the last seen frame dimensions, and whether the frame was stored in a texture. 2. When the encoder is configured the first time (WebRtcVideoSendStream::SetCodec()) - the last seen frame dimensions are used instead of the default dimensions. 3. A flag that indicates if encoding is to be done from a texture has been added to the webrtc::VideoStream and webrtc::VideoCodec structs, and it's been wired up to be passed down all the way to the JNI code in androidmediaencoder_jni.cc. 4. MediaCodecVideoEncoder::InitEncode is now reading the is_surface flag from the VideoCodec structure instead of guessing the default as false. This way we end up with the correct encoder configuration the first time around. 5. WebRtcVideoSendStream now takes an optimistic guess and requests non- rotated frames when the supported RtpExtensions list is not available. This makes the "early" frames arrive non-rotated, and the cached dimensions will be correct for the common case when the rotation extension is supported. If the other side is an older endpoint which does not support rotation, the encoder will have to be reconfigured - but it's better to penalize the uncommon case rather than the common one. Committed: https://crrev.com/3abb7644001d264c402184705950111d3fb8f181 Cr-Commit-Position: refs/heads/master@{#13173}

Patch Set 1 #

Patch Set 2 : Optimistically guess rotation is supported #

Total comments: 8

Patch Set 3 : Code review feedback #

Total comments: 15

Patch Set 4 : More CR feedback; replaced Dimensions with VideoFrameInfo #

Patch Set 5 : ReconfigureEncoderIfNecessary -> ReconfigureEncoder() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -47 lines) Patch
M webrtc/api/java/jni/androidmediaencoder_jni.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/common_types.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/config.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/config.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.h View 1 2 3 4 6 chunks +12 lines, -10 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.cc View 1 2 3 4 15 chunks +48 lines, -34 lines 0 comments Download
M webrtc/video/video_send_stream.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/video_frame.h View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (11 generated)
skvlad
4 years, 6 months ago (2016-06-14 23:09:13 UTC) #2
henrika_webrtc
Removed myself and pthatcher as reviewers. Added perkj@ instead. I don't know the video-related parts ...
4 years, 6 months ago (2016-06-15 08:14:03 UTC) #5
perkj_webrtc
nice. Just nits. https://codereview.webrtc.org/2067103002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2067103002/diff/20001/webrtc/common_types.h#newcode708 webrtc/common_types.h:708: bool encodeFromTexture; encode_from_texture please. We should ...
4 years, 6 months ago (2016-06-15 11:31:30 UTC) #7
skvlad
Added Tina and Tommi as reviewers for webrtc/config.h and webrtc/common_types.h. https://codereview.webrtc.org/2067103002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2067103002/diff/20001/webrtc/common_types.h#newcode708 ...
4 years, 6 months ago (2016-06-15 19:44:34 UTC) #10
perkj_webrtc
lgtm, Suggest mflodman as top level reviewer.
4 years, 6 months ago (2016-06-15 20:18:55 UTC) #12
pthatcher1
Just style and readability stuff. The logic seem sound. It's a great change, by the ...
4 years, 6 months ago (2016-06-15 20:40:23 UTC) #13
tommi
nice! lgtm once current comments have been addressed. https://codereview.webrtc.org/2067103002/diff/60001/webrtc/api/java/jni/androidmediaencoder_jni.cc File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2067103002/diff/60001/webrtc/api/java/jni/androidmediaencoder_jni.cc#newcode420 webrtc/api/java/jni/androidmediaencoder_jni.cc:420: codec_settings->encode_from_texture ...
4 years, 6 months ago (2016-06-15 21:00:39 UTC) #14
skvlad
https://codereview.webrtc.org/2067103002/diff/60001/webrtc/api/java/jni/androidmediaencoder_jni.cc File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2067103002/diff/60001/webrtc/api/java/jni/androidmediaencoder_jni.cc#newcode420 webrtc/api/java/jni/androidmediaencoder_jni.cc:420: codec_settings->encode_from_texture /* use_surface */)); On 2016/06/15 21:00:39, tommi-webrtc wrote: ...
4 years, 6 months ago (2016-06-15 22:10:36 UTC) #15
pthatcher1
lgtm
4 years, 6 months ago (2016-06-15 22:24:22 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067103002/90001
4 years, 6 months ago (2016-06-16 18:36:22 UTC) #19
commit-bot: I haz the power
Committed patchset #5 (id:90001)
4 years, 6 months ago (2016-06-16 19:08:09 UTC) #21
commit-bot: I haz the power
4 years, 6 months ago (2016-06-16 19:08:19 UTC) #23
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/3abb7644001d264c402184705950111d3fb8f181
Cr-Commit-Position: refs/heads/master@{#13173}

Powered by Google App Engine
This is Rietveld 408576698