Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1592883004:
Use high QP threshold for HW VP8 encoder frame downscaling. (Closed)
DescriptionUse high QP threshold for HW VP8 encoder frame downscaling.
Before HW VP8 downscaling was triggered by frame drops only.
Also reset the encoder when it drops large amount of frames in a row.
BUG=b/26504665
Committed: https://crrev.com/919ff753760ccb2f80c8120f808586ff9214a9f1
Cr-Commit-Position: refs/heads/master@{#11406}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments #
Total comments: 5
Patch Set 3 : Rename variable #Messages
Total messages: 29 (12 generated)
glaznev@webrtc.org changed reviewers: + pbos@webrtc.org
PTAL
mflodman@webrtc.org changed reviewers: + jackychen@webrtc.org
Adding Jacky as a reviewer since he write this originally and ha a KR to look at the QP thresholds during Q1.
I think Jacky should take a look as well. https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:82: #define MAX_DROPPED_FRAMES 60 ENCODER_STALL_FRAMEDROP_THRESHOLD maybe, something indicating that it's detecting stalls. https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:226: int frames_dropped_q_full_; // Number of dropped frames caused by full queue. consecutive_full_queue_frame_drops_ https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:376: const int kBadQpThreshold = kMaxQp - kMaxQp / 4; I think this should just be hard-coded as another value, max_qp / 4 doesn't really mean anything quantifyable, right?
Description was changed from ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops more than 70 frames in a raw. BUG=b/26504665 ========== to ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops more than 70 frames in a raw. BUG=b/26504665 ==========
jackychen@google.com changed reviewers: + jackychen@google.com - jackychen@webrtc.org
Description was changed from ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops more than 70 frames in a raw. BUG=b/26504665 ========== to ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops more than 70 frames in a row. BUG=b/26504665 ==========
https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:82: #define MAX_DROPPED_FRAMES 60 In the comment, "reset the encoder when it drops more than 70 frames in a raw", but 60 here? Maybe remove concrete number in the comment.
PTAL https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:82: #define MAX_DROPPED_FRAMES 60 On 2016/01/18 10:33:07, pbos-webrtc wrote: > ENCODER_STALL_FRAMEDROP_THRESHOLD maybe, something indicating that it's > detecting stalls. Done. https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:82: #define MAX_DROPPED_FRAMES 60 On 2016/01/19 02:21:01, jackychen1 wrote: > In the comment, "reset the encoder when it drops more than 70 frames in a raw", > but 60 here? Maybe remove concrete number in the comment. Done. https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:226: int frames_dropped_q_full_; // Number of dropped frames caused by full queue. On 2016/01/18 10:33:07, pbos-webrtc wrote: > consecutive_full_queue_frame_drops_ Done. https://codereview.webrtc.org/1592883004/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:376: const int kBadQpThreshold = kMaxQp - kMaxQp / 4; On 2016/01/18 10:33:07, pbos-webrtc wrote: > I think this should just be hard-coded as another value, max_qp / 4 doesn't > really mean anything quantifyable, right? Done.
Description was changed from ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops more than 70 frames in a row. BUG=b/26504665 ========== to ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops large amount of frames in a row. BUG=b/26504665 ==========
ping
glaznev@webrtc.org changed reviewers: + perkj@webrtc.org
glaznev@webrtc.org changed reviewers: + mflodman@webrtc.org - perkj@webrtc.org
On 2016/01/22 07:14:28, AlexG wrote: > ping Magnus, can you please review this CL - it is in review for 10 days already
On 2016/01/26 01:06:15, AlexG wrote: > On 2016/01/22 07:14:28, AlexG wrote: > > ping > > Magnus, can you please review this CL - it is in review for 10 days already Sorry, I thought Peter and Jacky would take the first pass on this since they have been working more on this than I have and I'm not very familiar with the details. Peter, Jacky, Can you take a look at this and I'll look at it in parallel?
Yes sorry for dropping the ball on this, let jackychen@ take a look as well since he's going to look into thresholds this quarter I think. lgtm with frames_dropped_ moved back into OnDroppedFrame, looks like it's called everywhere after dropping frames, right? https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:602: frames_dropped_++; move frames_dropped_ into OnDroppedFrame() ?
https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:379: kMaxQp / kLowQpThresholdDenominator, kBadQpThreshold, false); LGTM. You found framerate reduction bad for VP8 HW encoder or you want to do it after scaledown to the minimum resolution as you mentioned in one post? Since I will tune QP threshold also, do you want me to experiment and change in another CL?
https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:379: kMaxQp / kLowQpThresholdDenominator, kBadQpThreshold, false); On 2016/01/26 15:03:57, jackychen1 wrote: > LGTM. > > You found framerate reduction bad for VP8 HW encoder or you want to do it after > scaledown to the minimum resolution as you mentioned in one post? Since I will > tune QP threshold also, do you want me to experiment and change in another CL? It is not bad, but I thought there is less difference if we scale from 720p 30 fps to VGA 30 fps than when reducing 720p 30 fps to 15 fps. Scaling down resolution is not very visible for remote side, but frame rate reduction is quite visible - at least for me. Yes, I think we can start frame rate reduction only after we scale down to minimum possible resolution or may be at some resolution close to minimum - may be QVGA? This is something worth to experiment. QP thresholds are preliminary, they can definitely be changed in follow up CL. https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:602: frames_dropped_++; On 2016/01/26 14:48:57, pbos-webrtc wrote: > move frames_dropped_ into OnDroppedFrame() ? OnDroppedFrame is an override - I think it may be called by some other module, so frames_dropped_ will be incremented by other module. Originally I thought frames_dropped_ statistics to count frame drops initiated by Android specific implementation only - like input queue overflow or encoder falling behind.
https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:602: frames_dropped_++; On 2016/01/26 20:34:18, AlexG wrote: > On 2016/01/26 14:48:57, pbos-webrtc wrote: > > move frames_dropped_ into OnDroppedFrame() ? > > OnDroppedFrame is an override - I think it may be called by some other module, > so frames_dropped_ will be incremented by other module. Originally I thought > frames_dropped_ statistics to count frame drops initiated by Android specific > implementation only - like input queue overflow or encoder falling behind. Should we rename frames_dropped_ to frames_dropped_before_media_codec_ or something if it's not useful to know all dropped frames?
On 2016/01/27 15:55:49, pbos-webrtc wrote: > https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... > File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): > > https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... > talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:602: frames_dropped_++; > On 2016/01/26 20:34:18, AlexG wrote: > > On 2016/01/26 14:48:57, pbos-webrtc wrote: > > > move frames_dropped_ into OnDroppedFrame() ? > > > > OnDroppedFrame is an override - I think it may be called by some other module, > > so frames_dropped_ will be incremented by other module. Originally I thought > > frames_dropped_ statistics to count frame drops initiated by Android specific > > implementation only - like input queue overflow or encoder falling behind. > > Should we rename frames_dropped_ to frames_dropped_before_media_codec_ or > something if it's not useful to know all dropped frames? Done
On 2016/01/27 15:55:49, pbos-webrtc wrote: > https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... > File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): > > https://codereview.webrtc.org/1592883004/diff/20001/talk/app/webrtc/java/jni/... > talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:602: frames_dropped_++; > On 2016/01/26 20:34:18, AlexG wrote: > > On 2016/01/26 14:48:57, pbos-webrtc wrote: > > > move frames_dropped_ into OnDroppedFrame() ? > > > > OnDroppedFrame is an override - I think it may be called by some other module, > > so frames_dropped_ will be incremented by other module. Originally I thought > > frames_dropped_ statistics to count frame drops initiated by Android specific > > implementation only - like input queue overflow or encoder falling behind. > > Should we rename frames_dropped_ to frames_dropped_before_media_codec_ or > something if it's not useful to know all dropped frames? Done
The CQ bit was checked by glaznev@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from jackychen@google.com, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1592883004/#ps40001 (title: "Rename variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1592883004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592883004/40001
Message was sent while issue was closed.
Description was changed from ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops large amount of frames in a row. BUG=b/26504665 ========== to ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops large amount of frames in a row. BUG=b/26504665 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops large amount of frames in a row. BUG=b/26504665 ========== to ========== Use high QP threshold for HW VP8 encoder frame downscaling. Before HW VP8 downscaling was triggered by frame drops only. Also reset the encoder when it drops large amount of frames in a row. BUG=b/26504665 Committed: https://crrev.com/919ff753760ccb2f80c8120f808586ff9214a9f1 Cr-Commit-Position: refs/heads/master@{#11406} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/919ff753760ccb2f80c8120f808586ff9214a9f1 Cr-Commit-Position: refs/heads/master@{#11406} |
