Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672793002:
Change rtc::VideoSinkWants to have target and a max pixel count (Closed)
|
Created:
3 years, 10 months ago by sprang_webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, mflodman, kthelgason Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChange rtc::VideoSinkWants to have target and a max pixel count
The current method with max_pixel_count and max_pixel_count_step_up,
where only one should be used at a time and this first signaling an
inclusive upper bound and other other an exclusive lower bound, makes
for a lot of confusion.
I've updated this to have a desired target and a maximum instead. The
source should select a resolution as close to the target as possible,
but no higher than the maximum.
I intend to also add similar frame rate settings in an upcoming cl.
BUG=webrtc:4172, webrtc:6850
Review-Url: https://codereview.webrtc.org/2672793002
Cr-Commit-Position: refs/heads/master@{#16533}
Committed: https://chromium.googlesource.com/external/webrtc/+/84a3759825ec3bcba104196148be15aabc258a08
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comment #
Total comments: 14
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Minor tweaks #
Total comments: 5
Patch Set 5 : Clarified test #
Total comments: 2
Patch Set 6 : nit #Patch Set 7 : Fixed incorrect behavior in VideoAdapter, updated test #
Total comments: 8
Messages
Total messages: 53 (25 generated)
The CQ bit was checked by sprang@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/...
sprang@webrtc.org changed reviewers: + nisse@webrtc.org
Nisse, you were involved in the initial sink wants discussion. Do you wanna have a first look?
Comments on the semantics and merging of wants. Haven't yet looked at the details where this is used for adaptation. https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videobroadc... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videobroadc... webrtc/media/base/videobroadcaster.cc:103: wants.target_pixel_count = Optional<int>(); Wouldn't it be more natural in this case to clip the target, wants.target_pixel_count = wants.max_pixel_count; ? https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videosource... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videosource... webrtc/media/base/videosourceinterface.h:36: rtc::Optional<int> target_pixel_count; Using this to step up has the potential failure mode that we set a target_pixel_count higher than the current count, but the current count happens to be the closest possible so the source makes no change. It would make sense to me to also have a min_pixel_count, which you could set to just slighly above the current count to force a change. Precendence in case of conflicts, from higher to lower, would be max_pixel_count min_pixel_count target_pixel_count I think that should be expressive enough. But if it works for now without min_pixel_count, there's no need to add it now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videobroadc... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videobroadc... webrtc/media/base/videobroadcaster.cc:103: wants.target_pixel_count = Optional<int>(); On 2017/02/02 15:34:02, nisse-webrtc wrote: > Wouldn't it be more natural in this case to clip the target, > > wants.target_pixel_count = wants.max_pixel_count; > > ? Yes, I think so. The outcome won't change, but changed it in order to be more readable. https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videosource... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/2672793002/diff/1/webrtc/media/base/videosource... webrtc/media/base/videosourceinterface.h:36: rtc::Optional<int> target_pixel_count; On 2017/02/02 15:34:02, nisse-webrtc wrote: > Using this to step up has the potential failure mode that we set a > target_pixel_count higher than the current count, but the current count happens > to be the closest possible so the source makes no change. > > It would make sense to me to also have a min_pixel_count, which you could set to > just slighly above the current count to force a change. Precendence in case of > conflicts, from higher to lower, would be > > max_pixel_count > min_pixel_count > target_pixel_count > > I think that should be expressive enough. But if it works for now without > min_pixel_count, there's no need to add it now. Agree that a lower bound would be good as well. For now it works, since VideoAdapter is, as far as I can tell, the only implementation using the sinkwants to scale. I omitted min_pixel_count in order to not have to change too many things at once. I might add it before long, depending on how the framerate limits turn out. Good enough for now?
lgtm https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:70: // Multiply by 2/3. I think this logic deserves some explanation. But it's not new in this cl. https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:153: // TODO(kthelgason): remove the - |step_up_| hack when we change how Drop this TODO? https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:255: target_pixel_count.value_or(std::numeric_limits<int>::max()); You could swap order, and make max_pixel_count the default for target_pixel_count. https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:92: // Select the minimum requested target_pixel_count, if any, of all sinks so Sinks could use max_pixel_count to express a limit. So one could use average or median instead of minimum, right? Probably best to stick to minimum for now, though, if that's closest to the old behavior. Ultimately, we'd like to support separate dimensions for each sink, possibly pushing actual scaling further down the pipeline. https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:620: if (quality_scaler_ && qp >= 0) Is this fix related to the other changes? https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:716: // Don't request lower resolution if the current resolution is not lower When do you expect this to be true? When the source hasn't yet acted on the previous request, or if it isn't able to produce a smaller resolution? Might work slightly differently for steps up and down, because max_pixel_count has higher precedence. For step down, it might be clearer to write the test as something like if (current_pixel_count > wants.max_pixel.count.value_or(MAXINT)) /* No further action until source honors our max_pixel_count setting */ return; Corresponding logic for step up (forther below) is more fuzzy. I think current version is ok, but perhaps you could have a second look to see if the logic can be made clearer.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
cc kthelgasson since he has done a lot of work here and may have comments.
Description was changed from ========== Change rtc::VideSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inlcusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar framerate settings in an upcoming cl. BUG=webrtc:4172 ========== to ========== Change rtc::VideSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inlcusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar framerate settings in an upcoming cl. BUG=webrtc:4172 ==========
stefan@webrtc.org changed reviewers: - stefan@webrtc.org
+magjed for webrtc/media +stefan for webrtc/call/call_perf_tests.cc https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:70: // Multiply by 2/3. On 2017/02/06 08:43:19, nisse-webrtc wrote: > I think this logic deserves some explanation. But it's not new in this cl. I'll add a brief comment. But yes, it was not immediately obvious. :) https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:153: // TODO(kthelgason): remove the - |step_up_| hack when we change how On 2017/02/06 08:43:19, nisse-webrtc wrote: > Drop this TODO? Done. https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:255: target_pixel_count.value_or(std::numeric_limits<int>::max()); On 2017/02/06 08:43:18, nisse-webrtc wrote: > You could swap order, and make max_pixel_count the default for > target_pixel_count. Done. https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:92: // Select the minimum requested target_pixel_count, if any, of all sinks so On 2017/02/06 08:43:19, nisse-webrtc wrote: > Sinks could use max_pixel_count to express a limit. So one could use average or > median instead of minimum, right? > > Probably best to stick to minimum for now, though, if that's closest to the old > behavior. > > Ultimately, we'd like to support separate dimensions for each sink, possibly > pushing actual scaling further down the pipeline. I wasn't aware of the full context of this behavior so tried to stick close to the previous behavior. Agree that it could make sense to do something "smarter" if we can express the limit with max, but let's not do that in another cl in that case. I'll add a todo. https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:620: if (quality_scaler_ && qp >= 0) On 2017/02/06 08:43:19, nisse-webrtc wrote: > Is this fix related to the other changes? This is to prevent the quality scaler from using -1 values (indicating not available) when calculating the average qp. It resulted in triggering surprising adaptation requests with the fake encoder used in the test I added. I'm pretty sure we don't want this behavior from potential external decoders that don't report qp as well. +kthelgason can maybe comment on this? https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:716: // Don't request lower resolution if the current resolution is not lower On 2017/02/06 08:43:19, nisse-webrtc wrote: > When do you expect this to be true? When the source hasn't yet acted on the > previous request, or if it isn't able to produce a smaller resolution? > > Might work slightly differently for steps up and down, because max_pixel_count > has higher precedence. > > For step down, it might be clearer to write the test as something like > > if (current_pixel_count > wants.max_pixel.count.value_or(MAXINT)) > /* No further action until source honors our max_pixel_count setting */ > return; > > Corresponding logic for step up (forther below) is more fuzzy. > > I think current version is ok, but perhaps you could have a second look to see > if the logic can be made clearer. When the source hasn't yet acted on the previous request, so we don't want to request a new step needlessly. Looking direcly at the current max in the sink wants can maybe slightly simplify this, but would require exposure of that struct in VideoSourceProxy. Not sure if that's worth it? I tried to keep the behavior as close as possible to the previous. The old implementation stored the max and max_step values just to compare to the current for this purpose. It only populated one value at a time though so it could infer the direction of the step from that.
kthelgason@webrtc.org changed reviewers: + kthelgason@webrtc.org
https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (left): https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:133: static_cast<int>(step_up_)); good riddance! https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (right): https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:57: rtc::Optional<int> max_pixel_count); Maybe we should use this opportunity and change these arguments to just const int*? From the rtc::Optional docstring: When you want to pass a value of a type T that can fail to be there, const T* is almost always both fastest and cleanest. (If you're *sure* that the the caller will always already have an Optional<T>, const Optional<T>& is slightly faster than const T*, but this is a micro-optimization. In general, stick to const T*.) https://codereview.webrtc.org/2672793002/diff/40001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2672793002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:225: bool adapt_down_; Can we make this an enum instead? IMO having explicit direction = kDown or kUp is clearer than a boolean.
Addressed comments. kthelgason, any comment on this? https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (left): https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:133: static_cast<int>(step_up_)); On 2017/02/06 12:13:20, kthelgason wrote: > good riddance! :) https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (right): https://codereview.webrtc.org/2672793002/diff/40001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:57: rtc::Optional<int> max_pixel_count); On 2017/02/06 12:13:20, kthelgason wrote: > Maybe we should use this opportunity and change these arguments to just const > int*? > > From the rtc::Optional docstring: > When you want to pass a value of a > type T that can fail to be there, const T* is almost always both fastest and > cleanest. (If you're *sure* that the the caller will always already have an > Optional<T>, const Optional<T>& is slightly faster than const T*, but this is a > micro-optimization. In general, stick to const T*.) Since we're always gonna have an rtc::Optional in the sink wants, I think we should stick to that. I will however change this method to accept const rtc::Optional<int>& instead. Didn't even notice it wasn't already so. https://codereview.webrtc.org/2672793002/diff/40001/webrtc/video/vie_encoder.h File webrtc/video/vie_encoder.h (right): https://codereview.webrtc.org/2672793002/diff/40001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.h:225: bool adapt_down_; On 2017/02/06 12:13:21, kthelgason wrote: > Can we make this an enum instead? IMO having explicit direction = kDown or kUp > is clearer than a boolean. Done.
lgtm https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:620: if (quality_scaler_ && qp >= 0) On 2017/02/06 10:34:02, språng wrote: > On 2017/02/06 08:43:19, nisse-webrtc wrote: > > Is this fix related to the other changes? > > This is to prevent the quality scaler from using -1 values (indicating not > available) when calculating the average qp. It resulted in triggering surprising > adaptation requests with the fake encoder used in the test I added. > I'm pretty sure we don't want this behavior from potential external decoders > that don't report qp as well. > +kthelgason can maybe comment on this? SGTM. As part of the quality scaler work I tried to make sure all encoders populate |encoded_frame.qp| but I didn't consider the test encoders and external implementations. I think this is good to have as a safeguard.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:495: // to a higher. It's hard to understand why target_pixel_count means increase resolution. Should it be named differently to make it clear that it can't be used to target a lower resolution?
https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:495: // to a higher. On 2017/02/06 13:39:49, stefan-webrtc wrote: > It's hard to understand why target_pixel_count means increase resolution. Should > it be named differently to make it clear that it can't be used to target a lower > resolution? As far as I understand, it could be used to target a lower resolution, but it would be more advisory than using the max_pixel_count property.
https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2672793002/diff/20001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:620: if (quality_scaler_ && qp >= 0) On 2017/02/06 13:31:05, kthelgason wrote: > On 2017/02/06 10:34:02, språng wrote: > > On 2017/02/06 08:43:19, nisse-webrtc wrote: > > > Is this fix related to the other changes? > > > > This is to prevent the quality scaler from using -1 values (indicating not > > available) when calculating the average qp. It resulted in triggering > surprising > > adaptation requests with the fake encoder used in the test I added. > > I'm pretty sure we don't want this behavior from potential external decoders > > that don't report qp as well. > > +kthelgason can maybe comment on this? > > SGTM. As part of the quality scaler work I tried to make sure all encoders > populate |encoded_frame.qp| but I didn't consider the test encoders and external > implementations. I think this is good to have as a safeguard. Acknowledged. https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:495: // to a higher. On 2017/02/06 13:42:40, nisse-webrtc wrote: > On 2017/02/06 13:39:49, stefan-webrtc wrote: > > It's hard to understand why target_pixel_count means increase resolution. > Should > > it be named differently to make it clear that it can't be used to target a > lower > > resolution? > > As far as I understand, it could be used to target a lower resolution, but it > would be more advisory than using the max_pixel_count property. That's correct. I guess this is a more an integration test of how vie_encoder requests new resolutions, rather than a test of the sink wants mechanism itself.
https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:495: // to a higher. On 2017/02/06 13:49:53, språng wrote: > On 2017/02/06 13:42:40, nisse-webrtc wrote: > > On 2017/02/06 13:39:49, stefan-webrtc wrote: > > > It's hard to understand why target_pixel_count means increase resolution. > > Should > > > it be named differently to make it clear that it can't be used to target a > > lower > > > resolution? > > > > As far as I understand, it could be used to target a lower resolution, but it > > would be more advisory than using the max_pixel_count property. > > That's correct. I guess this is a more an integration test of how vie_encoder > requests new resolutions, rather than a test of the sink wants mechanism itself. Maybe update the comment to make that clear, i.e., "Lower resolution is requested by a VideoSendStream by setting..."
lgtm with comments updated.
Also fix spelling error in description.
Description was changed from ========== Change rtc::VideSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inlcusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar framerate settings in an upcoming cl. BUG=webrtc:4172 ========== to ========== Change rtc::VideoSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inclusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar frame rate settings in an upcoming cl. BUG=webrtc:4172 ==========
https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2672793002/diff/60001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:495: // to a higher. On 2017/02/06 14:01:28, stefan-webrtc wrote: > On 2017/02/06 13:49:53, språng wrote: > > On 2017/02/06 13:42:40, nisse-webrtc wrote: > > > On 2017/02/06 13:39:49, stefan-webrtc wrote: > > > > It's hard to understand why target_pixel_count means increase resolution. > > > Should > > > > it be named differently to make it clear that it can't be used to target a > > > lower > > > > resolution? > > > > > > As far as I understand, it could be used to target a lower resolution, but > it > > > would be more advisory than using the max_pixel_count property. > > > > That's correct. I guess this is a more an integration test of how vie_encoder > > requests new resolutions, rather than a test of the sink wants mechanism > itself. > > Maybe update the comment to make that clear, i.e., > > "Lower resolution is requested by a VideoSendStream by setting..." Done.
sprang@webrtc.org changed reviewers: + magjed@webrtc.org
actually +magjed for webrtc/media
Description was changed from ========== Change rtc::VideoSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inclusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar frame rate settings in an upcoming cl. BUG=webrtc:4172 ========== to ========== Change rtc::VideoSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inclusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar frame rate settings in an upcoming cl. BUG=webrtc:4172,webrtc:6850 ==========
webrtc/media looks good to me. I also think the current rtc::VideoSinkWants interface is confusing and I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=6850 for it (I added this issue to the BUG= field). Maybe you should include perkj@ and pbos@ to this CL because they had strong opinions about this. https://codereview.webrtc.org/2672793002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2672793002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:62: int min_pixel_diff = input_pixels; Can you do this instead? int min_pixel_diff = numeric_limits<int>:max();
Thanks, I'll comment in the bug you linked. https://codereview.webrtc.org/2672793002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/2672793002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.cc:62: int min_pixel_diff = input_pixels; On 2017/02/07 15:02:40, magjed_webrtc wrote: > Can you do this instead? > int min_pixel_diff = numeric_limits<int>:max(); Sure. Won't change the behavior but might be more readable.
lgtm
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, kthelgason@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2672793002/#ps100001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22610)
There was a miss in VideoAdapter::OnResolutionRequest(). It doesn't make sense to keep the old value for target bitrate in case the new request isn't set. It should be capped by the max instead. Updated incorrect test. wanna take another pass?
The CQ bit was checked by sprang@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 2017/02/10 13:01:54, språng wrote: > There was a miss in VideoAdapter::OnResolutionRequest(). It doesn't make sense > to keep the old value for target bitrate in case the new request isn't set. It > should be capped by the max instead. > > Updated incorrect test. > > wanna take another pass? Still lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kthelgason@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2672793002/#ps120001 (title: "Fixed incorrect behavior in VideoAdapter, updated test")
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": 120001, "attempt_start_ts": 1486738853751220,
"parent_rev": "e9ad271db4e0e2b3df8b21ca93e727c24838ffcf", "commit_rev":
"84a3759825ec3bcba104196148be15aabc258a08"}
Message was sent while issue was closed.
Description was changed from ========== Change rtc::VideoSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inclusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar frame rate settings in an upcoming cl. BUG=webrtc:4172,webrtc:6850 ========== to ========== Change rtc::VideoSinkWants to have target and a max pixel count The current method with max_pixel_count and max_pixel_count_step_up, where only one should be used at a time and this first signaling an inclusive upper bound and other other an exclusive lower bound, makes for a lot of confusion. I've updated this to have a desired target and a maximum instead. The source should select a resolution as close to the target as possible, but no higher than the maximum. I intend to also add similar frame rate settings in an upcoming cl. BUG=webrtc:4172,webrtc:6850 Review-Url: https://codereview.webrtc.org/2672793002 Cr-Commit-Position: refs/heads/master@{#16533} Committed: https://chromium.googlesource.com/external/webrtc/+/84a3759825ec3bcba10419614... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/84a3759825ec3bcba10419614...
Message was sent while issue was closed.
perkj@webrtc.org changed reviewers: + perkj@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:192: sink_wants_.target_pixel_count = rtc::Optional<int>(); nit: isn the word target_pixel_count here equally confusing? Naming is hard and I dont have a strong prefens either or. https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:208: // to be at most twice the number of pixels. nope, you have limited to be 4* times the number of pixels. https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:620: if (quality_scaler_ && qp >= 0) correct cl? Does this fix a bug? https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:710: !last_frame_info_) { Looks like a bug if last_frame_info_ = nullptr. How can we adapt unless a frame have been received.
Message was sent while issue was closed.
Thanks for you comments perkj@, uploaded https://codereview.webrtc.org/2690023002 to address them. https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:192: sink_wants_.target_pixel_count = rtc::Optional<int>(); On 2017/02/13 08:48:03, perkj_webrtc wrote: > nit: isn the word target_pixel_count here equally confusing? Naming is hard > and I dont have a strong prefens either or. I think it makes sense, since we want the source to return frames at a resolution as close as possible to target_pixel_count, rather than "a step up from X, regardless of the step size". Since we control the step size down, it makes sense to also try to control the step size up, imo. https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:208: // to be at most twice the number of pixels. On 2017/02/13 08:48:03, perkj_webrtc wrote: > nope, you have limited to be 4* times the number of pixels. This whole comment is misleading. I'll put up CL to clarify it. https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:620: if (quality_scaler_ && qp >= 0) On 2017/02/13 08:48:03, perkj_webrtc wrote: > correct cl? Does this fix a bug? Yes, if fixes a bug but it only seems to be triggered by FakeEncoder, which emits frames with qp = -1. It caused the test I added to fail because of incorrect qp scaling. See earlier comment. https://codereview.webrtc.org/2672793002/diff/120001/webrtc/video/vie_encoder... webrtc/video/vie_encoder.cc:710: !last_frame_info_) { On 2017/02/13 08:48:03, perkj_webrtc wrote: > Looks like a bug if last_frame_info_ = nullptr. How can we adapt unless a frame > have been received. I think some test triggered that. I may want to figure out why and fix the test + DCHECK here instead. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
