Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1351693005:
Read the number of TLs for VP9 too + cleanup (Closed)
|
Created:
5 years, 3 months ago by ivica Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, andresp, perkj_webrtc, mflodman, sprang_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRead the number of TLs for VP9 too + cleanup
In video_sender.cc, properly read the number of temporal layers for VP9 too.
Also, some cleanup in video_loopback.cc and video_quality_test.h.
Committed: https://crrev.com/c7199c2d0b4a70dedc5bb9df5d8d5c15ec17c156
Cr-Commit-Position: refs/heads/master@{#10201}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressing comments #Patch Set 3 : Removing unrelated comment #Patch Set 4 : rebase master and reordering of params #
Total comments: 10
Patch Set 5 : Addressing comments #
Total comments: 6
Patch Set 6 : Addressing comments #Patch Set 7 : rebase master #
Total comments: 1
Patch Set 8 : Use field trial #
Total comments: 2
Patch Set 9 : rebase master #Patch Set 10 : Reverting all frame dropping related changes #
Messages
Total messages: 49 (17 generated)
ivica@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1351693005/diff/1/webrtc/modules/video_coding/m... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/1/webrtc/modules/video_coding/m... webrtc/modules/video_coding/main/source/video_sender.cc:110: ? sendCodec->codecSpecific.VP9.numberOfTemporalLayers This "int numLayers = ..." was written before VP9.numberOfTemporalLayers existed. I assume 1 should be replaced now by this. https://codereview.webrtc.org/1351693005/diff/1/webrtc/video/video_loopback.cc File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/1/webrtc/video/video_loopback.c... webrtc/video/video_loopback.cc:18: #include "webrtc/video/video_quality_test.h" Making the headers look the same as in video/screenshare_loopback.cc
pbos@webrtc.org changed reviewers: + stefan@webrtc.org
+R stefan, what do you think about these changes overall? https://codereview.webrtc.org/1351693005/diff/1/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/1/webrtc/config.h#newcode109 webrtc/config.h:109: bool force_disable_wrapper_frame_dropper; Put this under a struct {} test; https://codereview.webrtc.org/1351693005/diff/1/webrtc/modules/video_coding/m... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/1/webrtc/modules/video_coding/m... webrtc/modules/video_coding/main/source/video_sender.cc:110: ? sendCodec->codecSpecific.VP9.numberOfTemporalLayers On 2015/09/22 11:15:36, ivica wrote: > This "int numLayers = ..." was written before VP9.numberOfTemporalLayers > existed. I assume 1 should be replaced now by this. Check if codecType is kVideoCodecVP9 before using it.
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, no build URL) android_dbg on tryserver.webrtc (JOB_FAILED, no build URL)
https://codereview.webrtc.org/1351693005/diff/1/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/1/webrtc/config.h#newcode109 webrtc/config.h:109: bool force_disable_wrapper_frame_dropper; On 2015/09/22 11:23:54, pbos-webrtc wrote: > Put this under a struct {} test; Done. https://codereview.webrtc.org/1351693005/diff/1/webrtc/modules/video_coding/m... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/1/webrtc/modules/video_coding/m... webrtc/modules/video_coding/main/source/video_sender.cc:110: ? sendCodec->codecSpecific.VP9.numberOfTemporalLayers On 2015/09/22 11:23:54, pbos-webrtc wrote: > On 2015/09/22 11:15:36, ivica wrote: > > This "int numLayers = ..." was written before VP9.numberOfTemporalLayers > > existed. I assume 1 should be replaced now by this. > > Check if codecType is kVideoCodecVP9 before using it. Done.
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4644) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/8393) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/173) mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/4307)
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1351693005/diff/60001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/common_types.h#new... webrtc/common_types.h:708: bool forceDisableWrapperFrameDropper; Maybe disableGenericBitrateFrameDropper? https://codereview.webrtc.org/1351693005/diff/60001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/config.h#newcode118 webrtc/config.h:118: } test; Maybe we can have this as a non-test option? It isn't unreasonable to think that someone might want to do this, although it's not recommended. https://codereview.webrtc.org/1351693005/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/video_sender.cc:114: : 1); Please write this in a more readable way. https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... File webrtc/video/screenshare_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... webrtc/video/screenshare_loopback.cc:88: "Disable FrameDropper in the wrapper. " Generic bitrate frame dropper?
https://codereview.webrtc.org/1351693005/diff/60001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/common_types.h#new... webrtc/common_types.h:708: bool forceDisableWrapperFrameDropper; On 2015/10/01 08:45:59, stefan-webrtc (holmer) wrote: > Maybe disableGenericBitrateFrameDropper? Renamed. https://codereview.webrtc.org/1351693005/diff/60001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/config.h#newcode118 webrtc/config.h:118: } test; On 2015/10/01 08:45:59, stefan-webrtc (holmer) wrote: > Maybe we can have this as a non-test option? It isn't unreasonable to think that > someone might want to do this, although it's not recommended. Removed the 'test' struct. https://codereview.webrtc.org/1351693005/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/video_sender.cc:114: : 1); On 2015/10/01 08:45:59, stefan-webrtc (holmer) wrote: > Please write this in a more readable way. Done. Used ifs instead. https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... File webrtc/video/screenshare_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... webrtc/video/screenshare_loopback.cc:88: "Disable FrameDropper in the wrapper. " On 2015/10/01 08:45:59, stefan-webrtc (holmer) wrote: > Generic bitrate frame dropper? Renamed. Not sure how to name the flag though. Renamed from disable_wrapper_frame_dropper to disable_frame_dropper to keep it short.
lgtm https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... File webrtc/video/screenshare_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... webrtc/video/screenshare_loopback.cc:88: "Disable FrameDropper in the wrapper. " On 2015/10/01 09:29:20, ivica wrote: > On 2015/10/01 08:45:59, stefan-webrtc (holmer) wrote: > > Generic bitrate frame dropper? > > Renamed. Not sure how to name the flag though. Renamed from > disable_wrapper_frame_dropper to disable_frame_dropper to keep it short. remove "in the wrapper". No one using this application will know what that means https://codereview.webrtc.org/1351693005/diff/80001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/80001/webrtc/config.h#newcode116 webrtc/config.h:116: bool disable_generic_bitrate_frame_dropper; Add a comment on which further describes this frame dropper, e.g., "Disables the generic frame dropper which job it is to drop frames if the encoder produces a bitrate which is above the target bitrate." https://codereview.webrtc.org/1351693005/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/video_sender.cc:110: if (sendCodec->codecType == kVideoCodecVP8) {} on all if/else/else ifs since when it's multiple lines. https://codereview.webrtc.org/1351693005/diff/80001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/80001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:121: "Disable generic bitrate frame dropper in the wrapper. " - in the wrapper
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1351693005/#ps100001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/100001
https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... File webrtc/video/screenshare_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/60001/webrtc/video/screenshare_... webrtc/video/screenshare_loopback.cc:88: "Disable FrameDropper in the wrapper. " On 2015/10/01 09:58:04, stefan-webrtc (holmer) wrote: > On 2015/10/01 09:29:20, ivica wrote: > > On 2015/10/01 08:45:59, stefan-webrtc (holmer) wrote: > > > Generic bitrate frame dropper? > > > > Renamed. Not sure how to name the flag though. Renamed from > > disable_wrapper_frame_dropper to disable_frame_dropper to keep it short. > > remove "in the wrapper". No one using this application will know what that means Done. https://codereview.webrtc.org/1351693005/diff/80001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/80001/webrtc/config.h#newcode116 webrtc/config.h:116: bool disable_generic_bitrate_frame_dropper; On 2015/10/01 09:58:04, stefan-webrtc (holmer) wrote: > Add a comment on which further describes this frame dropper, e.g., > > "Disables the generic frame dropper which job it is to drop frames if the > encoder produces a bitrate which is above the target bitrate." Done. https://codereview.webrtc.org/1351693005/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/video_sender.cc:110: if (sendCodec->codecType == kVideoCodecVP8) On 2015/10/01 09:58:04, stefan-webrtc (holmer) wrote: > {} on all if/else/else ifs since when it's multiple lines. Done. https://codereview.webrtc.org/1351693005/diff/80001/webrtc/video/video_loopba... File webrtc/video/video_loopback.cc (right): https://codereview.webrtc.org/1351693005/diff/80001/webrtc/video/video_loopba... webrtc/video/video_loopback.cc:121: "Disable generic bitrate frame dropper in the wrapper. " On 2015/10/01 09:58:04, stefan-webrtc (holmer) wrote: > - in the wrapper Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1047)
ivica@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman, could you please take a look at the changes in common_types.h, config.h and config.cc? pbos and stefan already reviewed the CL, but they are not the owners of those files.
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If this is only for testing, wouldn't it make sens to use FiledTrial implementation instead? https://codereview.webrtc.org/1351693005/diff/120001/webrtc/config.h File webrtc/config.h (right): https://codereview.webrtc.org/1351693005/diff/120001/webrtc/config.h#newcode116 webrtc/config.h:116: // disables the generic frame dropper which job it is to drop frames if the s/disables/Disables
On 2015/10/06 10:49:15, mflodman wrote: > If this is only for testing, wouldn't it make sens to use FiledTrial > implementation instead? > > https://codereview.webrtc.org/1351693005/diff/120001/webrtc/config.h > File webrtc/config.h (right): > > https://codereview.webrtc.org/1351693005/diff/120001/webrtc/config.h#newcode116 > webrtc/config.h:116: // disables the generic frame dropper which job it is to > drop frames if the > s/disables/Disables I think that makes sense. Can you look into ScopedFieldTrials to set something globally for just this test? webrtc/test/field_trial.h:39: explicit ScopedFieldTrials(const std::string& config);
> I think that makes sense. Can you look into ScopedFieldTrials to set something > globally for just this test? > > webrtc/test/field_trial.h:39: explicit ScopedFieldTrials(const std::string& > config); You mean to keep the -disable_frame_dropper flag in video/video_loopback.cc and video/screenshare_loopback.cc, so that those flags override the field trial? Isn't it enough just to use -force_fieldtrials=WebRTC-DisableGenericBitrateFrameDropper/Enabled/? (I've uploaded the new patch, it is very short now). Yeah, do you have any suggestions for the name? I kept the old one Stefan suggested.
This solution works for me too.
LGTM as long as Peter and Stefan is ok with this change too.
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/160001
Do you have a test that intends to use this as well? It feels weird to approve a feature that isn't used. https://codereview.webrtc.org/1351693005/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/video_sender.cc:122: (field_trial::FindFullName("WebRTC-DisableGenericBitrateFrameDropper") Can we call this WebRTC-Test- or so? I'd like to distinguish between things that we test with and things that we intend to launch. WDYT?
https://codereview.webrtc.org/1351693005/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/video_sender.cc (right): https://codereview.webrtc.org/1351693005/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/video_sender.cc:122: (field_trial::FindFullName("WebRTC-DisableGenericBitrateFrameDropper") On 2015/10/07 11:27:55, pbos-webrtc wrote: > Can we call this WebRTC-Test- or so? I'd like to distinguish between things that > we test with and things that we intend to launch. WDYT? Now when it's so simple, I'm not sure if it's necessary to commit it. Still, for the same reason it's not a big deal. I was asked to generate stats and graphs with the frame dropper disabled, and the good thing of keeping it here is that the script I used to generate all the graphs will work without modifying the code (script is not in the repository). I don't know what's best.
On 2015/10/07 11:32:53, ivica wrote: > https://codereview.webrtc.org/1351693005/diff/140001/webrtc/modules/video_cod... > File webrtc/modules/video_coding/main/source/video_sender.cc (right): > > https://codereview.webrtc.org/1351693005/diff/140001/webrtc/modules/video_cod... > webrtc/modules/video_coding/main/source/video_sender.cc:122: > (field_trial::FindFullName("WebRTC-DisableGenericBitrateFrameDropper") > On 2015/10/07 11:27:55, pbos-webrtc wrote: > > Can we call this WebRTC-Test- or so? I'd like to distinguish between things > that > > we test with and things that we intend to launch. WDYT? > > Now when it's so simple, I'm not sure if it's necessary to commit it. Still, for > the same reason it's not a big deal. > I was asked to generate stats and graphs with the frame dropper disabled, and > the good thing of keeping it here is that the script I used to generate all the > graphs will work without modifying the code (script is not in the repository). > I don't know what's best. Should there be code for generating these stats and graphs without the framedropper? If so it's not dead code.
I've completely removed all frame dropping related changes, as that one line doesn't make much sense to be uploaded without the script that uses it. Leaving that for later. Still, there are few very small changes in this CL that would be good to commit.
lgtm
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1351693005/#ps180001 (title: "Reverting all frame dropping related changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1351693005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1351693005/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c7199c2d0b4a70dedc5bb9df5d8d5c15ec17c156 Cr-Commit-Position: refs/heads/master@{#10201} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
