Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578123002:
Add QP stats to the statsview in AppRTCMobile for ios. (Closed)
|
Created:
4 years ago by daniela-webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd QP stats to the statsview in AppRTCMobile for ios.
The following QP stats are added:
*avgQP for the current polling interval as a fraction of the current delta sum over the current delta of encoded frames
*total QPSum
*total number of encoded frames
BUG=NONE
Review-Url: https://codereview.webrtc.org/2578123002
Cr-Commit-Position: refs/heads/master@{#15669}
Committed: https://chromium.googlesource.com/external/webrtc/+/b6c456b7721529cea59078273bba365d64548825
Patch Set 1 #Patch Set 2 : Fix format #
Total comments: 4
Patch Set 3 : Remove delta ivars and improve wording #
Total comments: 4
Patch Set 4 : Add explicit comparison and set AvgQP to 0 when encoded frames is 0. #Messages
Total messages: 16 (6 generated)
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@chromium.org
Please take a look. Thanks!
This will be really useful, thanks! Some small comments :) https://codereview.webrtc.org/2578123002/diff/20001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m (right): https://codereview.webrtc.org/2578123002/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:50: int _deltaFramesEncoded; I don't think the _delta* variables need to be stored explicitly, as they can always be computed from the other two. We should try to keep as little state as possible IMO. https://codereview.webrtc.org/2578123002/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:110: "QP %d | (QPSum)%d| (Frames encoded)%d\n "; I'm not sure we ever care about the QPSum and frames encoded. Maybe we should avoid cluttering up the HUD and just keep average QP? Also, please write "average QP (last x frames)" or something.
denicija@webrtc.org changed reviewers: + magjed@webrtc.org - magjed@chromium.org
https://codereview.webrtc.org/2578123002/diff/20001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m (right): https://codereview.webrtc.org/2578123002/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:50: int _deltaFramesEncoded; On 2016/12/16 09:41:37, kthelgason wrote: > I don't think the _delta* variables need to be stored explicitly, as they can > always be computed from the other two. We should try to keep as little state as > possible IMO. Done. https://codereview.webrtc.org/2578123002/diff/20001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:110: "QP %d | (QPSum)%d| (Frames encoded)%d\n "; On 2016/12/16 09:41:37, kthelgason wrote: > I'm not sure we ever care about the QPSum and frames encoded. Maybe we should > avoid cluttering up the HUD and just keep average QP? Also, please write > "average QP (last x frames)" or something. Done.
lgtm
https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m (right): https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:167: return deltaFramesEncoded ? deltaQPSum / deltaFramesEncoded : deltaQPSum; I prefer explicit comparison with 0, i.e. 'deltaFramesEncoded > 0'. https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:167: return deltaFramesEncoded ? deltaQPSum / deltaFramesEncoded : deltaQPSum; Is deltaQPSum meaningful when deltaFramesEncoded==0?
https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m (right): https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:167: return deltaFramesEncoded ? deltaQPSum / deltaFramesEncoded : deltaQPSum; On 2016/12/16 12:33:17, magjed_webrtc wrote: > Is deltaQPSum meaningful when deltaFramesEncoded==0? Valid point. What would make more sense for avgQP in that case? -1?
https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m (right): https://codereview.webrtc.org/2578123002/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/ARDStatsBuilder.m:167: return deltaFramesEncoded ? deltaQPSum / deltaFramesEncoded : deltaQPSum; On 2016/12/16 13:06:19, denicija-webrtc wrote: > On 2016/12/16 12:33:17, magjed_webrtc wrote: > > Is deltaQPSum meaningful when deltaFramesEncoded==0? > > Valid point. What would make more sense for avgQP in that case? -1? 0 is also fine. I think it will be 0 with the current code, but then I prefer an explicit 0.
lgtm
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2578123002/#ps60001 (title: "Add explicit comparison and set AvgQP to 0 when encoded frames is 0.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482141158871830,
"parent_rev": "c3e1cabc696240e4b5a128653264785292878205", "commit_rev":
"b6c456b7721529cea59078273bba365d64548825"}
Message was sent while issue was closed.
Description was changed from ========== Add QP stats to the statsview in AppRTCMobile for ios. The following QP stats are added: *avgQP for the current polling interval as a fraction of the current delta sum over the current delta of encoded frames *total QPSum *total number of encoded frames BUG=NONE ========== to ========== Add QP stats to the statsview in AppRTCMobile for ios. The following QP stats are added: *avgQP for the current polling interval as a fraction of the current delta sum over the current delta of encoded frames *total QPSum *total number of encoded frames BUG=NONE Review-Url: https://codereview.webrtc.org/2578123002 Cr-Commit-Position: refs/heads/master@{#15669} Committed: https://chromium.googlesource.com/external/webrtc/+/b6c456b7721529cea59078273... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/b6c456b7721529cea59078273... |
