Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538913003:
Remove limit on how often quality scaling downscales. (Closed)
|
Created:
4 years ago by kthelgason Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, tterriberry_mozilla.com, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove limit on how often quality scaling downscales
When starting from 720p this is necessary to achieve acceptable
quality at low bitrates.
BUG=webrtc:6495
Committed: https://crrev.com/5e13d4112404531efb2244072e5ebe29a5a05214
Cr-Commit-Position: refs/heads/master@{#15356}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove quality downgrade limit #Patch Set 3 : Add lower scaling limit #Patch Set 4 : Add test for downscale limit #Patch Set 5 : size_t -> int #
Messages
Total messages: 34 (16 generated)
Description was changed from ========== Allow scaling up to 4 times for quality reasons. When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG= ========== to ========== Allow scaling up to 4 times for quality reasons. When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 ==========
kthelgason@webrtc.org changed reviewers: + magjed@webrtc.org, stefan@webrtc.org
1 character diff, should be quick!
Is this fixing a regression or an improvement? Assuming regression as it refers to the refactoring bug https://codereview.webrtc.org/2538913003/diff/1/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2538913003/diff/1/webrtc/video/vie_encoder.h#ne... webrtc/video/vie_encoder.h:66: // Downscale resolution at most 2 times for low-quality reasons. 4 times... :) Or remove that part of the comment... Does this look good independent of codec? I'd guess it should be independent, but it might be a good idea to check both vp8 and h.264 SW and HW?
On 2016/11/30 10:42:13, stefan-webrtc (holmer) wrote: > Is this fixing a regression or an improvement? Assuming regression as it refers > to the refactoring bug This is a regression. > Does this look good independent of codec? I'd guess it should be independent, > but it might be a good idea to check both vp8 and h.264 SW and HW? After some experimenting I found that it actually makes sense to just remove the limit. There is a built-in limit for how low an encoder will go, and if we find undesirable behaviour with some encoder this should be tuned in the QP thresholds instead. The main motivators for this decision are: 1: In really low bandwidth scenarios (~100kbps) the only way to get acceptable video is to scale down to a really low resolution 2: This means we can increase the starting resolution without worrying about updating this.
On 2016/11/30 12:05:35, kthelgason wrote: > On 2016/11/30 10:42:13, stefan-webrtc (holmer) wrote: > > Is this fixing a regression or an improvement? Assuming regression as it > refers > > to the refactoring bug > > This is a regression. > > > Does this look good independent of codec? I'd guess it should be independent, > > but it might be a good idea to check both vp8 and h.264 SW and HW? > > After some experimenting I found that it actually makes sense to just remove the > limit. There is a built-in limit for how low an encoder will go, and if we find > undesirable behaviour with some encoder this should be tuned in the QP > thresholds instead. > > The main motivators for this decision are: > 1: In really low bandwidth scenarios (~100kbps) the only way to get acceptable > video is to scale down to a really low resolution Are you sure about that? I would have expected there to be a threshold after which it's preferable to have quantization artifacts rather than super blurry video. > 2: This means we can increase the starting resolution without worrying about > updating this.
On 2016/11/30 12:09:46, stefan-webrtc (holmer) wrote: > Are you sure about that? I would have expected there to be a threshold after > which it's preferable to have quantization artifacts rather than super blurry > video. Yes, for every resolution there is a bitrate at which the codec artifacts become worse than dropping down to the next resolution down. This applies even to starting_resolution - n for any n. The job of the quality scaler is to scale down at this threshold. We won't scale down infinitely, the encoder has a lower limit, and I think it makes more sense to specify the bottom value rather than an offset from the top, especially when we don't know what the top is.
On 2016/11/30 12:15:35, kthelgason wrote: > On 2016/11/30 12:09:46, stefan-webrtc (holmer) wrote: > > Are you sure about that? I would have expected there to be a threshold after > > which it's preferable to have quantization artifacts rather than super blurry > > video. > > Yes, for every resolution there is a bitrate at which the codec artifacts become > worse than dropping down to the next resolution down. This applies even to > starting_resolution - n for any n. The job of the quality scaler is to scale > down at this threshold. We won't scale down infinitely, the encoder has a lower > limit, and I think it makes more sense to specify the bottom value rather than > an offset from the top, especially when we don't know what the top is. Yes, agree that it makes more sense to have an absolute limit rather than something relative to an unknown resolution. What are the limits set by our encoders today? I just want to make sure we don't end up scaling down to something like 16x9 pixels or similar... :)
On 2016/11/30 12:38:46, stefan-webrtc (holmer) wrote: > Yes, agree that it makes more sense to have an absolute limit rather than > something relative to an unknown resolution. > > What are the limits set by our encoders today? I just want to make sure we don't > end up scaling down to something like 16x9 pixels or similar... :) Yeah of course. It turns out we don't actually have a limit in pixels, but the lowest scale factor is 3/16 of the input res. So starting from VGA we go down to 120/90 in the worst case. I've not actually seen this happen though, even with a 100kbps bitrate limit and shaking the camera vigorously. This is defined here: https://cs.chromium.org/chromium/src/third_party/webrtc/media/base/videoadapt...
On 2016/11/30 12:46:39, kthelgason wrote: > On 2016/11/30 12:38:46, stefan-webrtc (holmer) wrote: > > Yes, agree that it makes more sense to have an absolute limit rather than > > something relative to an unknown resolution. > > > > What are the limits set by our encoders today? I just want to make sure we > don't > > end up scaling down to something like 16x9 pixels or similar... :) > > Yeah of course. It turns out we don't actually have a limit in pixels, but the > lowest scale factor is 3/16 of the input res. So starting from VGA we go down to > 120/90 in the worst case. > I've not actually seen this happen though, even with a 100kbps bitrate limit and > shaking the camera vigorously. > > This is defined here: > https://cs.chromium.org/chromium/src/third_party/webrtc/media/base/videoadapt... Ok, seems like that is also a change compared to how this worked when the QualityScaler did the scaling by itself, right? I think we should document somewhere that we're now relying on the video adapter to not allow scaling down too far. It seems a bit brittle, since it could easily happen that the adapter is improved to support any resolution, which then changes the behavior here. What scale factors do we allow with textures, the same? I would have preferred to make this a bit more explicit, since we believe there's a threshold when scaling further isn't good.
Description was changed from ========== Allow scaling up to 4 times for quality reasons. When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 ========== to ========== Remove limit on how often quality scaling downscales When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 ==========
On 2016/11/30 12:55:48, stefan-webrtc (holmer) wrote: > Ok, seems like that is also a change compared to how this worked when the > QualityScaler did the scaling by itself, right? Kind of. We used to have a minimum resolution, and the quality scaler would calculate the number of steps it could scale based on that. > I think we should document somewhere that we're now relying on the video adapter > to not allow scaling down too far. It seems a bit brittle, since it could easily > happen that the adapter is improved to support any resolution, which then > changes the behavior here. What scale factors do we allow with textures, the > same? > > I would have preferred to make this a bit more explicit, since we believe > there's a threshold when scaling further isn't good. We can add a lower threshold in the "RequestResolutionLowerThan" method. I think that's much better than having a limited number of steps, and would ensure that this doesn't go crazy if the scale factors are changed.
On 2016/11/30 12:59:57, kthelgason wrote: > On 2016/11/30 12:55:48, stefan-webrtc (holmer) wrote: > > Ok, seems like that is also a change compared to how this worked when the > > QualityScaler did the scaling by itself, right? > > Kind of. We used to have a minimum resolution, and the quality scaler would > calculate the number of steps it could scale based on that. > > > I think we should document somewhere that we're now relying on the video > adapter > > to not allow scaling down too far. It seems a bit brittle, since it could > easily > > happen that the adapter is improved to support any resolution, which then > > changes the behavior here. What scale factors do we allow with textures, the > > same? > > > > I would have preferred to make this a bit more explicit, since we believe > > there's a threshold when scaling further isn't good. > > We can add a lower threshold in the "RequestResolutionLowerThan" method. I think > that's much better than having a limited number of steps, and would ensure that > this doesn't go crazy if the scale factors are changed. Sounds like a good option!
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please take another look. The limit was determined pretty arbitrarily, and it was hard to get the Quality Scaler to actually go down that far, even at 60kbps I had to really shake the camera. But it seems fine IMO, and I think it's better that we have a product that works even at those super-low bitrates.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
I wouldn't cry if we had a test for this. Otherwise this looks good :)
lgtm
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/11/30 14:09:17, stefan-webrtc (holmer) wrote: > I wouldn't cry if we had a test for this. > > Otherwise this looks good :) I definitely wouldn't either. I've added one now.
Thanks, lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/3768)
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2538913003/#ps70001 (title: "size_t -> int")
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": 70001, "attempt_start_ts": 1480592007759290,
"parent_rev": "e810adecf34ee4eee9619f16263127f4a0c5f7eb", "commit_rev":
"bbdf847655164cee22fa11e125dff2807362d10f"}
Message was sent while issue was closed.
Description was changed from ========== Remove limit on how often quality scaling downscales When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 ========== to ========== Remove limit on how often quality scaling downscales When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Remove limit on how often quality scaling downscales When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 ========== to ========== Remove limit on how often quality scaling downscales When starting from 720p this is necessary to achieve acceptable quality at low bitrates. BUG=webrtc:6495 Committed: https://crrev.com/5e13d4112404531efb2244072e5ebe29a5a05214 Cr-Commit-Position: refs/heads/master@{#15356} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5e13d4112404531efb2244072e5ebe29a5a05214 Cr-Commit-Position: refs/heads/master@{#15356} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
