Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1695263002:
Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. (Closed)
|
Created:
4 years, 10 months ago by perkj_webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMove direct use of VideoCapturer::VideoAdapter to VideoSinkWants.
The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2.
This cl change CPU adaptation to use a new VideoSinkWants.Resolution
Tested on a N5 with hw acceleration turned off.
BUG=webrtc:5426
Committed: https://crrev.com/2d5f0913f25a9e504eec2c2ecbb4e7647fd283b4
Cr-Commit-Position: refs/heads/master@{#11804}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added asyncproxy.h for discussion. #Patch Set 3 : Added from_width && from_height in resolution change req. #
Total comments: 3
Patch Set 4 : WIP #Patch Set 5 : switched to int. Found bug in adapter... #
Total comments: 17
Patch Set 6 : Tried to address pthatchers comments. #
Total comments: 1
Patch Set 7 : Something like this? #
Total comments: 6
Patch Set 8 : Got unittests to pass. Updated to design doc suggestions. #Patch Set 9 : Rebased #Patch Set 10 : Added unittest for changig resolution #Patch Set 11 : Added unit test to test what happens when resolution changes. Rebased #Patch Set 12 : cl format + lint + added videobroadcaster_unittest #Patch Set 13 : Changed name to wants.max_pixel_count_step_up #Patch Set 14 : rebased #
Total comments: 18
Patch Set 15 : Addressed comments. Narrowed scope of |lock_| #Patch Set 16 : Fixed wants.rotation_applied = false + fixed unittests. #
Total comments: 5
Patch Set 17 : Removed proxy - fixed bug in videoadapter. #Patch Set 18 : Rebased #Patch Set 19 : Removed test for screencast from videocapturer #
Total comments: 25
Patch Set 20 : Addressed pbos comments #
Total comments: 2
Patch Set 21 : fixed nit #Patch Set 22 : Rebased #Messages
Total messages: 38 (10 generated)
perkj@webrtc.org changed reviewers: + nisse@webrtc.org, pthatcher@webrtc.org
Hi- new cl for discussion. Can you please take a look and say what you think before I spend too much time moving this in one or the other direction? This cl currently does not intend to change the current behavior an uses the CoordinatedVideoAdapter as is. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1971: info.adapt_changes += capturer_->video_adapter()->adaptation_changes(); where do we want this type of stats to take place? GetStats is for the peerconnection so I guess it make sence to keep it here and not move it to the source implementation. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:997: // WebRtcVideoSendStream creates a copy and later change the configuration. remove comments. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1919: capturer_->AddOrUpdateSink(this, wants); This will actually currently not work since OnLoadUpdate is called on an internal thread in webrtc video engine. I think this call and similar calls should be marshaled to the worker thread and the lock removed and replaced with a thread check. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1920: } Also- Would it make more sense to store an absolute resolution in the wants and move the logic from the CoordindatedVideoAdapter to calculate the resolution here? ?
Description was changed from ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. BUG=webrtc:5426 ========== to ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 ==========
perkj@webrtc.org changed reviewers: + pbos@webrtc.org
Also adding pbos https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1971: info.adapt_changes += capturer_->video_adapter()->adaptation_changes(); On 2016/02/15 12:28:00, perkj_webrtc wrote: > where do we want this type of stats to take place? GetStats is for the > peerconnection so I guess it make sence to keep it here and not move it to the > source implementation. Now added a proxy. See comments in asyncproxy.h https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:997: // WebRtcVideoSendStream creates a copy and later change the configuration. On 2016/02/15 12:28:00, perkj_webrtc wrote: > remove comments. Done.
Can you please take a look at this cl? I added the resolution to change from in VideoSinkWants and is pretty happy with the result. wdyt? Unit tests has not been fully updated / added.
I had some draft comments on an earlier version, sorry I forgot to publish them earlier. I think I can guess what the new from_width and from_height are for, but they seem a bit kludgy to me. The next question is, where does the logic really belong, to process the "load" (actually, encode time) measurement, and decide which resolution should be used? To me, it makes sense to make that decision in the sendstream, or maybe rtpsender, and put the *result* of the decision into VideoSinkWants. Which would then carry the desired resolution (or scale factor or min/max, those are later details). I understand you might want to keep the video adapter (which I haven't looked into) unchanged in this cl, and that's perfectly fine with me, but if the current from_width things are an interim solution to achieve that, we need to also have the direction clear. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/base/videosource... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/base/videosource... webrtc/media/base/videosourceinterface.h:32: Resolution resolution = Resolution::KEEP; I don't think it's a good idea to have actions here. Specifying a "want" should be an idempotent operation, so that repeated AddOrUpdateSink with the same sink and the same wants should be harmless. To me, it would make sense to have max_width and max_height, or if for some reason that's not appropriate, I could live with a scale factor. Choice mainly depends on the desired action if the source changes resolution spontaneously: Should we keep scaling with the same factor (then the "wants" should be that scale factor), or should we aim for giving the sink an unchanged resolution (then max_width and max_height is a better fit)? https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1920: } On 2016/02/15 12:28:00, perkj_webrtc wrote: > Also- Would it make more sense to store an absolute resolution in the wants and > move the logic from the CoordindatedVideoAdapter to calculate the resolution > here? ? I think so. (Or a scale factor, also with the logic to bump it up and down moved here). https://codereview.webrtc.org/1695263002/diff/40001/webrtc/media/base/asyncpr... File webrtc/media/base/asyncproxy.h (right): https://codereview.webrtc.org/1695263002/diff/40001/webrtc/media/base/asyncpr... webrtc/media/base/asyncproxy.h:16: // TODO(perkj): Now before landing -- decide if this make sence and if it make I din't think there's any point in doing some macrology for this until there's three or so uses of it. I guess it might also be possible to templatify things by having an Observer template parameterized by the OnUpdate argument type, and then use even more templating, instead of the preprocessor, for proxy objects? About the filename, do you intend to add declarations for other proxies in the same file? https://codereview.webrtc.org/1695263002/diff/40001/webrtc/media/base/videoso... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1695263002/diff/40001/webrtc/media/base/videoso... webrtc/media/base/videosourceinterface.h:31: struct ResolutionRequest { This needs a comment. At first look, it seems a bit too complex.
ok- if you can - please take another look to check what this is going. I am working on updating existing unittests. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/base/videosource... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/base/videosource... webrtc/media/base/videosourceinterface.h:32: Resolution resolution = Resolution::KEEP; On 2016/02/17 08:19:46, nisse-webrtc wrote: > I don't think it's a good idea to have actions here. Specifying a "want" should > be an idempotent operation, so that repeated AddOrUpdateSink with the same sink > and the same wants should be harmless. > > To me, it would make sense to have max_width and max_height, or if for some > reason that's not appropriate, I could live with a scale factor. > > Choice mainly depends on the desired action if the source changes resolution > spontaneously: Should we keep scaling with the same factor (then the "wants" > should be that scale factor), or should we aim for giving the sink an unchanged > resolution (then max_width and max_height is a better fit)? After discussions- we don't want to change aspect ratio - so decided on setting max_number_of_pixels. https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:1920: } On 2016/02/17 08:19:46, nisse-webrtc wrote: > On 2016/02/15 12:28:00, perkj_webrtc wrote: > > Also- Would it make more sense to store an absolute resolution in the wants > and > > move the logic from the CoordindatedVideoAdapter to calculate the resolution > > here? ? > > I think so. (Or a scale factor, also with the logic to bump it up and down moved > here). Moved logic from CoordinatedVideoAdapter to here. I don't want to change behavior in this cl. https://codereview.webrtc.org/1695263002/diff/40001/webrtc/media/base/videoso... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1695263002/diff/40001/webrtc/media/base/videoso... webrtc/media/base/videosourceinterface.h:31: struct ResolutionRequest { On 2016/02/17 08:19:46, nisse-webrtc wrote: > This needs a comment. At first look, it seems a bit too complex. Changed to only store max_number_of_pixels for now. Maybe we will need min_number_of_pixels in the future.
https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (left): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:135: float process_threshold() const { return process_threshold_; } Do we not need all this anymore, or did it get moved? https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:36: current_wants_.max_number_of_pixels = std::numeric_limits<int>::max(); Would it make more sense to use an Optional<int>? https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:44: } We should document that a local preview sink/renderer will cause the full resolution to be sent from a track. So, for this to affect encoding, we need to make sure that the adapter is attached down the pipeline after the local preview is attached to the track. I'm not so sure this CL puts the adapter in the right place. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.h (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.h:55: size_t height_ = 0; Why are these need? And, we should probably call it last_frame_width_, last_frame_height_. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1502: Can you explain why this proxy is necessary? https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1976: } This doesn't seem to work right for situations where the frame size changes on its own, like with a screencast being resized. For example: start at 1080p handle overuse, set max to 1080/2 = 720p screencast changes on its own to 360p handle non-overuse, set max to 360p*2 = 720p. Wouldn't it make more sense to do: sink_wants_.max_number_of_pixels *= 2; or sink_wants_.max_number_of_pixels /= 2; ? Also, instead of recording the count of the number of times we've gone down, I think it would make sense to record the amount we've scaled it down (1, 2, 4), and have a max for that (4). In fact, what I think we need to do is: 1. Have the adapter here, near the encoder. 2. Have the adapter act like a source and a sink, and the encoder attaches to it as a source, and it attaches to the main source as a sink. Then, adaptation happens even if local preview is attached to the original video track at the beginning of the pipeline.
https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (left): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:135: float process_threshold() const { return process_threshold_; } On 2016/02/17 23:05:56, pthatcher1 wrote: > Do we not need all this anymore, or did it get moved? void OnCpuResolutionRequest(AdaptRequest request) and void OnOutputFormatRequest(const VideoFormat& format) seems to be the only one we use. I added void OnLimitResolution(int max_number_of_pixels) and removed OnCpuResolutionRequest and removed void OnCpuLoadUpdated(int current_cpus, int max_cpus, float process_load, float system_load); But then all those other setters and getters did not do anything so I removed them. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:36: current_wants_.max_number_of_pixels = std::numeric_limits<int>::max(); On 2016/02/17 23:05:56, pthatcher1 wrote: > Would it make more sense to use an Optional<int>? In the interface yes.. but maybe the implementation got worse.. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:44: } On 2016/02/17 23:05:56, pthatcher1 wrote: > We should document that a local preview sink/renderer will cause the full > resolution to be sent from a track. So, for this to affect encoding, we need to > make sure that the adapter is attached down the pipeline after the local > preview is attached to the track. > > I'm not so sure this CL puts the adapter in the right place. This is not intended to change the current behavior or move the adapter. ie - adaptation affect all sinks - including local preview. This selects max_number_of_pixels based on the the sink with lowest requirements. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.h (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.h:55: size_t height_ = 0; On 2016/02/17 23:05:56, pthatcher1 wrote: > Why are these need? > > And, we should probably call it last_frame_width_, last_frame_height_. They are not needed... left overs from a previous iteration of the patch. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1502: On 2016/02/17 23:05:56, pthatcher1 wrote: > Can you explain why this proxy is necessary? OnLoad is called on a media engine thread. Not the worker thread. So it is to not have to make the videobroadcaster have to take care of AddOrUpdateSink calls from multiple threads + probably in the near future - accept frames to be delivered on a third thread. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1976: } On 2016/02/17 23:05:56, pthatcher1 wrote: > This doesn't seem to work right for situations where the frame size changes on > its own, like with a screencast being resized. > > For example: > > start at 1080p > handle overuse, set max to 1080/2 = 720p > screencast changes on its own to 360p > handle non-overuse, set max to 360p*2 = 720p. > > Wouldn't it make more sense to do: > > sink_wants_.max_number_of_pixels *= 2; > or > sink_wants_.max_number_of_pixels /= 2; > > ? > > > Also, instead of recording the count of the number of times we've gone down, I > think it would make sense to record the amount we've scaled it down (1, 2, 4), > and have a max for that (4). > > In fact, what I think we need to do is: > > 1. Have the adapter here, near the encoder. > 2. Have the adapter act like a source and a sink, and the encoder attaches to > it as a source, and it attaches to the main source as a sink. Then, adaptation > happens even if local preview is attached to the original video track at the > beginning of the pipeline. Currently - CPU adaptation is not done for screen cast - I added the check in VideoCapturer instead of as it was (See old WebRtcVideoChannel2::OnLoadUpdate(Load load). I can move the logic back here if you want. But from your example- isn't that exactly what we want to express here: The load for a certain resolution is either too high or too low. We would like to get the max_number_of_pixels as an argument to OnLoad so that we know what resolution the decision was based on but this is as good guess as we can currently do. Alternatively we can have a scale factor or just forwarw Up /Down in the wants (See patchset 1). But that gets klunky since that express a request for a change not a want. Nisse did not like it and I agree. An adapter does not necessarily know how to scale a native handle currently. That can be solved by adding a scale factor to the video frames as we discussed and actually let the encoder do the scaling. But that is a bit larger change that I think we should do down the line. This currently work since the adapter causes the frame factory in the capture implementation to create the correct sized frames from the beginning. Regardless - I think it make sense to notify the source of what resolution the sinks wants. In the future we can decide to reconfigure the camera to the resolution that the sinks are actually using. Currently adaptation happens even if local preview is attached, that is no change from how it is now. The source produce the minimum resolution requested by all sinks. I also think that make sence for CPU adaptation - doing scaling early saves CPU for all sinks instead of them doing scaling individually.
https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (left): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:135: float process_threshold() const { return process_threshold_; } On 2016/02/18 13:22:46, perkj_webrtc wrote: > On 2016/02/17 23:05:56, pthatcher1 wrote: > > Do we not need all this anymore, or did it get moved? > > void OnCpuResolutionRequest(AdaptRequest request) and > void OnOutputFormatRequest(const VideoFormat& format) seems to be the only one > we use. > I added void OnLimitResolution(int max_number_of_pixels) and removed > OnCpuResolutionRequest and removed void OnCpuLoadUpdated(int current_cpus, int > max_cpus, float process_load, float system_load); > > But then all those other setters and getters did not do anything so I removed > them. Would it simplify things with a separate cl deleting the unused functions? https://codereview.webrtc.org/1695263002/diff/100001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/100001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:40: if (!current_wants_.max_number_of_pixels) { I liked using SIZE_T_MAX (or whatever it's called) for "no restriction" better, then this logic gets a lot simpler. Now, both unset and set to SIZE_T_MAX are possible, and with fully equivalent meaning.
https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1976: } On 2016/02/18 13:22:46, perkj_webrtc wrote: > On 2016/02/17 23:05:56, pthatcher1 wrote: > > This doesn't seem to work right for situations where the frame size changes on > > its own, like with a screencast being resized. > > > > For example: > > > > start at 1080p > > handle overuse, set max to 1080/2 = 720p > > screencast changes on its own to 360p > > handle non-overuse, set max to 360p*2 = 720p. > > > > Wouldn't it make more sense to do: > > > > sink_wants_.max_number_of_pixels *= 2; > > or > > sink_wants_.max_number_of_pixels /= 2; > > > > ? > > > > > > Also, instead of recording the count of the number of times we've gone down, I > > think it would make sense to record the amount we've scaled it down (1, 2, 4), > > and have a max for that (4). > > > > In fact, what I think we need to do is: > > > > 1. Have the adapter here, near the encoder. > > 2. Have the adapter act like a source and a sink, and the encoder attaches to > > it as a source, and it attaches to the main source as a sink. Then, > adaptation > > happens even if local preview is attached to the original video track at the > > beginning of the pipeline. > > Currently - CPU adaptation is not done for screen cast - I added the check in > VideoCapturer instead of as it was (See old > WebRtcVideoChannel2::OnLoadUpdate(Load load). I can move the logic back here if > you want. > > But from your example- isn't that exactly what we want to express here: The load > for a certain resolution is either too high or too low. We would like to get the > max_number_of_pixels as an argument to OnLoad so that we know what resolution > the decision was based on but this is as good guess as we can currently do. > Alternatively we can have a scale factor or just forwarw Up /Down in the wants > (See patchset 1). But that gets klunky since that express a request for a change > not a want. Nisse did not like it and I agree. > > An adapter does not necessarily know how to scale a native handle currently. > That can be solved by adding a scale factor to the video frames as we discussed > and actually let the encoder do the scaling. > But that is a bit larger change that I think we should do down the line. This > currently work since the adapter causes the frame factory in the capture > implementation to create the correct sized frames from the beginning. > > Regardless - I think it make sense to notify the source of what resolution the > sinks wants. In the future we can decide to reconfigure the camera to the > resolution that the sinks are actually using. > > Currently adaptation happens even if local preview is attached, that is no > change from how it is now. The source produce the minimum resolution requested > by all sinks. I also think that make sence for CPU adaptation - doing scaling > early saves CPU for all sinks instead of them doing scaling individually. > > So back to your example- pbos convinced me of your visdom in not expressing up / down in terms of max_number_of_pixels like this. How about changing the wants again to express Up / Down from a resolution?
Patchset #7 (id:120001) has been deleted
On 2016/02/18 14:30:54, perkj_webrtc wrote: > https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1976: } > On 2016/02/18 13:22:46, perkj_webrtc wrote: > > On 2016/02/17 23:05:56, pthatcher1 wrote: > > > This doesn't seem to work right for situations where the frame size changes > on > > > its own, like with a screencast being resized. > > > > > > For example: > > > > > > start at 1080p > > > handle overuse, set max to 1080/2 = 720p > > > screencast changes on its own to 360p > > > handle non-overuse, set max to 360p*2 = 720p. > > > > > > Wouldn't it make more sense to do: > > > > > > sink_wants_.max_number_of_pixels *= 2; > > > or > > > sink_wants_.max_number_of_pixels /= 2; > > > > > > ? > > > > > > > > > Also, instead of recording the count of the number of times we've gone down, > I > > > think it would make sense to record the amount we've scaled it down (1, 2, > 4), > > > and have a max for that (4). > > > > > > In fact, what I think we need to do is: > > > > > > 1. Have the adapter here, near the encoder. > > > 2. Have the adapter act like a source and a sink, and the encoder attaches > to > > > it as a source, and it attaches to the main source as a sink. Then, > > adaptation > > > happens even if local preview is attached to the original video track at the > > > beginning of the pipeline. > > > > Currently - CPU adaptation is not done for screen cast - I added the check in > > VideoCapturer instead of as it was (See old > > WebRtcVideoChannel2::OnLoadUpdate(Load load). I can move the logic back here > if > > you want. > > > > But from your example- isn't that exactly what we want to express here: The > load > > for a certain resolution is either too high or too low. We would like to get > the > > max_number_of_pixels as an argument to OnLoad so that we know what resolution > > the decision was based on but this is as good guess as we can currently do. > > Alternatively we can have a scale factor or just forwarw Up /Down in the wants > > (See patchset 1). But that gets klunky since that express a request for a > change > > not a want. Nisse did not like it and I agree. > > > > An adapter does not necessarily know how to scale a native handle currently. > > That can be solved by adding a scale factor to the video frames as we > discussed > > and actually let the encoder do the scaling. > > But that is a bit larger change that I think we should do down the line. This > > currently work since the adapter causes the frame factory in the capture > > implementation to create the correct sized frames from the beginning. > > > > Regardless - I think it make sense to notify the source of what resolution the > > sinks wants. In the future we can decide to reconfigure the camera to the > > resolution that the sinks are actually using. > > > > Currently adaptation happens even if local preview is attached, that is no > > change from how it is now. The source produce the minimum resolution requested > > by all sinks. I also think that make sence for CPU adaptation - doing scaling > > early saves CPU for all sinks instead of them doing scaling individually. > > > > > > So back to your example- pbos convinced me of your visdom in not expressing up / > down in terms of max_number_of_pixels like this. > How about changing the wants again to express Up / Down from a resolution? I updated with a new suggestion - where I leave the CoordinatedVideoAdapter unchanged. Back to Up/Down from number of pixels..... More cleaning is needed but is the suggestion acceptable?
https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... File webrtc/media/base/videoadapter.h (left): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/base/videoad... webrtc/media/base/videoadapter.h:135: float process_threshold() const { return process_threshold_; } On 2016/02/18 14:03:42, nisse-webrtc wrote: > On 2016/02/18 13:22:46, perkj_webrtc wrote: > > On 2016/02/17 23:05:56, pthatcher1 wrote: > > > Do we not need all this anymore, or did it get moved? > > > > void OnCpuResolutionRequest(AdaptRequest request) and > > void OnOutputFormatRequest(const VideoFormat& format) seems to be the only one > > we use. > > I added void OnLimitResolution(int max_number_of_pixels) and removed > > OnCpuResolutionRequest and removed void OnCpuLoadUpdated(int current_cpus, int > > max_cpus, float process_load, float system_load); > > > > But then all those other setters and getters did not do anything so I removed > > them. > > Would it simplify things with a separate cl deleting the unused functions? That's exactly what I was thinking. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1502: On 2016/02/18 13:22:46, perkj_webrtc wrote: > On 2016/02/17 23:05:56, pthatcher1 wrote: > > Can you explain why this proxy is necessary? > > OnLoad is called on a media engine thread. Not the worker thread. > So it is to not have to make the videobroadcaster have to take care of > AddOrUpdateSink calls from multiple threads + probably in the near future - > accept frames to be delivered on a third thread. It seems like a lot of complexity to add. And I think we need to allow AddOrUpdateSink to be callable from any thread anyway. Otherwise, the caller needs to know what thread is has to call it on, which I think would be a mess. Shouldn't that be pretty simple with a CriticalSection? Similarly, I think we should allow OnFrame to be called from any thread, and have a sink do a thread-hop or lock if it needs to. Several places we already do that, because frames can come from random threads on different mobile OSes. So, can we just make both OnFrame and AddOrUpdateSink thread-safe, and not have to deal with this proxy complexity? https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:39: bool sink_agrees_on_resolution = true; I think you mean sinks_agree_on_resolution https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:47: current_number_of_pixels_) { This would be more clear reversed as "if (current > wants)" than "if (wants < current)". https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:54: current_number_of_pixels_) { Same here: "if (wants < current)" would be more clear than "if (current > wants)". https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:101: current_number_of_pixels_ = frame.GetWidth() * frame.GetHeight(); I think it would be better to save this as last_frame_pixel_count_ https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:101: current_number_of_pixels_ = frame.GetWidth() * frame.GetHeight(); Since the pixel count may have changed, I think you have to re-calculate the current_wants_. https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1973: rtc::Optional<rtc::VideoSinkWants::ResolutionRequest>(request); I think I liked what we had better before :(. This just seems too complex. But let's step back for a moment an think of what we're trying to accomplish. We want the following: 1. When we detect CPU overuse at resolution X, start scaling down to resolution X/A. This can happen multiple times down to some limit of X/Z. 2. When we detect CPU underuse at resolution X/B, scale down less, to only X/A. This can happen multiple times all the way up to X/1 == X. 3. If the video source changes resolution (and perhaps aspect ratio) on its own, from X to Y, adjust the scale factor from A to B so that we have X/A =~ Y/B. If Y > X, then B > A. If Y < X, then B < A. Once we know the scale factor A, then the scaler has a really easy job: just scale with that factor. The simplest thing would be for the encoder sink to tell the source "I want frames scaled down by factor A". When overuse is detected, we scale down further (to B > A). When underuse is detected, we scale down less (to B < A). The video broadcaster simply chooses the sink with the largest factor. That is simple and easy. However, it has the downside that when the video source changes resolution from X to Y, the frame the encoder sink sees will change from X/A to Y/A. Then we'll have to wait until another overuse/underuse cycle to change Y/A to Y/B, which would be undesirable. A slight tweak to this would be for the encoder sink to tell the source "I want frames scaled down to X/A". When overuse is detected, we scale down further (to X/B < X/A). When underuse is detected, we scale down less (to X/B > X/A). The video broadcaster simply chooses the sink with the smallest scaled resolution. That is also simple and easy. However, it has the upside that when the video source changes resolutions from X to Y, the source can say to itself "I should scale down to Y/B == X/A". Then the encoder sink will see the resolution Y/B, which should be about the same as it wanted with X/A. And if it sees underuse or overuse again, it will tell the source "I want to see Z/C". And thus the big question is: how do we represent X/A? And here are the options: A. As scale factor A or B or C. Really easy, but doesn't react well to changing resolutions. However, because we don't adapt for screencast and we also change the source object when we switch cameras, I think we don't have a current use case for this. So, it would meet our current needs and be very simple. B. As pixel_count(X)/A, or scaled pixel count, or max_pixel_count. This is what you had before, and now that I think about it more, I think your code you had before is correct, and pretty simple. I'd like to hear pbos's objections to it. Like I said, at first glance, it seems better (much more simple) than this updated patch. C. As pixel_count + direction request, which you have here. This model seems more complex because it puts the source in charge of how much to go up or down and it makes the aggregation of wants so much more complicated. Can someone explain to me the advantage of option C over option B? By the way, I think this is equivalent to having two values in SinkWants: less_than_pixel_count and more_than_pixel_count, which I'd greatly prefer over the pixel_count + direction if we go with C. D. As resolution + scale. Basically "if the resolution is X, scale down by A". Then it's clear what the source should do (scale down by A), and if it does happen to change resolution (if we ever adapt for screencast, for example), then the source can figure out what to do. The aggregation might still be a bit complex, though. I think I prefer B > A > D > C.
As far as I got today. I have not looked at all comments but please take a look at the sent out document for why we would like to have a way of saying- increase the resolution by one step. Feedback on this cl is of course also very welcome. https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1502: On 2016/02/19 06:08:39, pthatcher1 wrote: > On 2016/02/18 13:22:46, perkj_webrtc wrote: > > On 2016/02/17 23:05:56, pthatcher1 wrote: > > > Can you explain why this proxy is necessary? > > > > OnLoad is called on a media engine thread. Not the worker thread. > > So it is to not have to make the videobroadcaster have to take care of > > AddOrUpdateSink calls from multiple threads + probably in the near future - > > accept frames to be delivered on a third thread. > > It seems like a lot of complexity to add. > > And I think we need to allow AddOrUpdateSink to be callable from any thread > anyway. Otherwise, the caller needs to know what thread is has to call it on, > which I think would be a mess. Shouldn't that be pretty simple with a > CriticalSection? > > Similarly, I think we should allow OnFrame to be called from any thread, and > have a sink do a thread-hop or lock if it needs to. Several places we already > do that, because frames can come from random threads on different mobile OSes. > > So, can we just make both OnFrame and AddOrUpdateSink thread-safe, and not have > to deal with this proxy complexity? > I agreee with OnFrame as said before. I don't so much like having AddOrUpdateSink and RemoveSink be called on arbitrary threads.. If AddOrUpdateSink is called on two threads for the same sink - it feels like you can end up in a weird state. Ex- CPU is low so the CPU monitor allows the sink to increase the bitrate. But the encoder thread say that the produced quality is awesome - so it thinks it can produce frames with a higher resolution and calls AddOrUpdateSink on another tread... Somewhere these two requests should be coordinated. I agree that is does not necessarily has to be here. AddOrUpdateSink for two separate sinks on two separate threads might make more sence. But it means that the source implementation must be thread safe too so that it can handle the changed sinkwants. Also- pbos pointed out a possible problem of holding |lock_| in webrtcvideosendstream::onload from a tread the webrtc engine. That might cause a possible deadlock when the webrtcvideosendstream is reconstructed.
On 2016/02/19 14:57:52, perkj_webrtc wrote: > As far as I got today. I have not looked at all comments but please take a look > at the sent out document for why we would like to have a way of saying- increase > the resolution by one step. > > Feedback on this cl is of course also very welcome. > > https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1695263002/diff/80001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1502: > On 2016/02/19 06:08:39, pthatcher1 wrote: > > On 2016/02/18 13:22:46, perkj_webrtc wrote: > > > On 2016/02/17 23:05:56, pthatcher1 wrote: > > > > Can you explain why this proxy is necessary? > > > > > > OnLoad is called on a media engine thread. Not the worker thread. > > > So it is to not have to make the videobroadcaster have to take care of > > > AddOrUpdateSink calls from multiple threads + probably in the near future - > > > accept frames to be delivered on a third thread. > > > > It seems like a lot of complexity to add. > > > > And I think we need to allow AddOrUpdateSink to be callable from any thread > > anyway. Otherwise, the caller needs to know what thread is has to call it > on, > > which I think would be a mess. Shouldn't that be pretty simple with a > > CriticalSection? > > > > Similarly, I think we should allow OnFrame to be called from any thread, and > > have a sink do a thread-hop or lock if it needs to. Several places we already > > do that, because frames can come from random threads on different mobile OSes. > > > > > So, can we just make both OnFrame and AddOrUpdateSink thread-safe, and not > have > > to deal with this proxy complexity? > > > > I agreee with OnFrame as said before. > > > I don't so much like having AddOrUpdateSink and RemoveSink be called on > arbitrary threads.. > > If AddOrUpdateSink is called on two threads for the same sink - it feels like > you can end up in a weird state. Ex- CPU is low so the CPU monitor allows the > sink to increase the bitrate. But the encoder thread say that the produced > quality is awesome - so it thinks it can produce frames with a higher resolution > and calls AddOrUpdateSink on another tread... Somewhere these two requests > should be coordinated. I agree that is does not necessarily has to be here. > > AddOrUpdateSink for two separate sinks on two separate threads might make more > sence. But it means that the source implementation must be thread safe too so > that it can handle the changed sinkwants. > > Also- pbos pointed out a possible problem of holding |lock_| in > webrtcvideosendstream::onload from a tread the webrtc engine. That might cause a > possible deadlock when the webrtcvideosendstream is reconstructed. The potential deadlock happens when VideoSendStream (inside webrtcvideosendstream) is reconstructed if OnLoad() takes the same lock that protects VideoSendStream (unless it's posted onto another thread that's separate from VideoSendStream).
pbos, ptatcher- Can you please help by talking directly to each other and resolve how you want these wants to work? All of the last proposals works for me. When that is resolved- I think this cl is ready for a review.
On 2016/02/22 11:20:51, perkj_webrtc wrote: > pbos, ptatcher- Can you please help by talking directly to each other and > resolve how you want these wants to work? All of the last proposals works for > me. > > When that is resolved- I think this cl is ready for a review. Thanks Peter and Peter. pbos- do you want to take a first look at this and see if I got it right?
The design looks good, just comments on the impl. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videoa... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videoa... webrtc/media/base/videoadapter.cc:513: OnCpuResolutionRequest(request); Could this be shortened to the following? if (max_pixel_count && *max_pixel_count < GetOutputNumPixels()) { OnCpuResolutionRequest(DOWNGRADE); } else if (max_pixel_count_step_up && *max_pixel_count_step_up <= GetOutputNumPixels()) { OnCpuResolutionRequest(UPGRADE); } https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:110: I think this would be more readable: VideoSinkWants wants; for (auto& sink : sinks_) { // wants.rotation_applied == ANY(sink.wants.rotation_applied) if (sink.wants.rotation_applied) { wants.rotation_applied = true; } // wants.max_pixel_count == MIN(sink.wants.max_pixel_count) if (sink.wants.max_pixel_count && (!wants.max_pixel_count || (sink_wants.max_pixel_count < wants.max_pixel_count))) { wants.max_pixel_count = sink.wants.max_pixel_count; } // wants.max_pixel_count_step_up == MIN(sink.wants.max_pixel_count_step_up) if (sink.wants.max_pixel_count_step_up && (!wants.max_pixel_count_step_up || (sink_wants.max_pixel_count_step_up < wants.max_pixel_count_step_up))) { wants.max_pixel_count_step_up = sink.wants.max_pixel_count_step_up; } } current_wants_ = wants; https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videos... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videos... webrtc/media/base/videosourceinterface.h:35: // should produce a resolution one "step" higher than this. Can we add a little to this? Perhaps like so: "Like max_pixel_count, but relative to the given value. The max should be one "step up" from the given value. In practice, this means the sink can consume this amount of pixels but wants more and the should produce a resolution one "step" higher than this, but not higher". https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1475: class WebRtcVideoChannel2::WebRtcVideoSendStream::LoadObserverProxy { This still looks too complex to me, but maybe I'm just not smart enough to understand it. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1996: last_dimensions_.height * last_dimensions_.width / 2); Why can't use you just do: sink_wants_.max_pixel_count = max_pixel_count; on this line? https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1996: last_dimensions_.height * last_dimensions_.width / 2); Shouldn't we clear the sink_wants_.max_pixel_count_step_up here? https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1999: cpu_adapted_ = false; It's still cpu_adapted sometimes, right? Just slightly less? Wouldn't we have to clear both max_pixel_counts to be sure it's not adapted any more? https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2001: last_dimensions_.height * last_dimensions_.width); Why not just call this max_pixel_count_step_up?
https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:110: On 2016/02/25 07:40:30, pthatcher1 wrote: > I think this would be more readable: > > VideoSinkWants wants; > for (auto& sink : sinks_) { > // wants.rotation_applied == ANY(sink.wants.rotation_applied) > if (sink.wants.rotation_applied) { > wants.rotation_applied = true; > } > // wants.max_pixel_count == MIN(sink.wants.max_pixel_count) > if (sink.wants.max_pixel_count && (!wants.max_pixel_count || > (sink_wants.max_pixel_count < wants.max_pixel_count))) { > wants.max_pixel_count = sink.wants.max_pixel_count; > } > // wants.max_pixel_count_step_up == MIN(sink.wants.max_pixel_count_step_up) > if (sink.wants.max_pixel_count_step_up && (!wants.max_pixel_count_step_up || > (sink_wants.max_pixel_count_step_up < wants.max_pixel_count_step_up))) { > wants.max_pixel_count_step_up = sink.wants.max_pixel_count_step_up; > } > } > > current_wants_ = wants; I think there's one more thing done in Per's version (which maybe needs a comment). If it turns out that MIN(max_pixel_step_up) >= MIN(max_pixel), then in the merged wants, max_pixel_step_up is left unset. The comparisons could also written as if (sink.wants.max_pixel_count.value_or(std::numeric_limits<int>::max()) < max_pixels) { max_pixels = *sink.wants.max_pixel_count; } Which would be elegant if it weren't for the numeric_limits thing being so lengthy. And as you can guess, I still find the rtc::Optional unnecessary, but I won't insist if you really like that interface.
PTAL https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videoa... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videoa... webrtc/media/base/videoadapter.cc:513: OnCpuResolutionRequest(request); On 2016/02/25 07:40:30, pthatcher1 wrote: > Could this be shortened to the following? > > if (max_pixel_count && *max_pixel_count < GetOutputNumPixels()) { > OnCpuResolutionRequest(DOWNGRADE); > } else if (max_pixel_count_step_up && *max_pixel_count_step_up <= > GetOutputNumPixels()) { > OnCpuResolutionRequest(UPGRADE); > } Done. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster.cc:110: On 2016/02/25 09:31:07, nisse-webrtc wrote: > On 2016/02/25 07:40:30, pthatcher1 wrote: > > I think this would be more readable: > > > > VideoSinkWants wants; > > for (auto& sink : sinks_) { > > // wants.rotation_applied == ANY(sink.wants.rotation_applied) > > if (sink.wants.rotation_applied) { > > wants.rotation_applied = true; > > } > > // wants.max_pixel_count == MIN(sink.wants.max_pixel_count) > > if (sink.wants.max_pixel_count && (!wants.max_pixel_count || > > (sink_wants.max_pixel_count < wants.max_pixel_count))) { > > wants.max_pixel_count = sink.wants.max_pixel_count; > > } > > // wants.max_pixel_count_step_up == MIN(sink.wants.max_pixel_count_step_up) > > if (sink.wants.max_pixel_count_step_up && (!wants.max_pixel_count_step_up || > > (sink_wants.max_pixel_count_step_up < wants.max_pixel_count_step_up))) { > > wants.max_pixel_count_step_up = sink.wants.max_pixel_count_step_up; > > } > > } > > > > current_wants_ = wants; > > I think there's one more thing done in Per's version (which maybe needs a > comment). If it turns out that MIN(max_pixel_step_up) >= MIN(max_pixel), then in > the merged wants, max_pixel_step_up is left unset. > > The comparisons could also written as > > if (sink.wants.max_pixel_count.value_or(std::numeric_limits<int>::max()) > < max_pixels) { > max_pixels = *sink.wants.max_pixel_count; > } > > Which would be elegant if it weren't for the numeric_limits thing being so > lengthy. And as you can guess, I still find the rtc::Optional unnecessary, but I > won't insist if you really like that interface. Added that condition afterwards and went with pthatchers suggestion. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videos... File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/base/videos... webrtc/media/base/videosourceinterface.h:35: // should produce a resolution one "step" higher than this. On 2016/02/25 07:40:30, pthatcher1 wrote: > Can we add a little to this? Perhaps like so: "Like max_pixel_count, but > relative to the given value. The max should be one "step up" from the given > value. In practice, this means the sink can consume this amount of pixels but > wants more and the should produce a resolution one "step" higher than this, but > not higher". Done. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1475: class WebRtcVideoChannel2::WebRtcVideoSendStream::LoadObserverProxy { On 2016/02/25 07:40:30, pthatcher1 wrote: > This still looks too complex to me, but maybe I'm just not smart enough to > understand it. I am sure that is not true. But I am not sure how do do this more to your liking. I think we need to post async to the worker thread. We can have SendStream (or this proxy) inherit MessageHandlerand do a normal Post directly in OnloadUpdate I assume and use Thread::Clear to remove anny unhandled messages on destruction. ie- no ref counting and no asyncinvoker. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1996: last_dimensions_.height * last_dimensions_.width / 2); On 2016/02/25 07:40:30, pthatcher1 wrote: > Why can't use you just do: > > sink_wants_.max_pixel_count = max_pixel_count; > > on this line? Done. https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1999: cpu_adapted_ = false; On 2016/02/25 07:40:30, pthatcher1 wrote: > It's still cpu_adapted sometimes, right? Just slightly less? Wouldn't we have > to clear both max_pixel_counts to be sure it's not adapted any more? Yes. This is just used for the info.adapt_reason stats. I can count the number of ups / downs and that might be slightly better. The problem might be screen share- that we don't adapt anyway.... Ie, say we adapt down twice and then up once and then the screen share window is enlarged outside our control and we don't get any more OnLoadUpdate. Are we then cpu constrained or not? https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2001: last_dimensions_.height * last_dimensions_.width); On 2016/02/25 07:40:30, pthatcher1 wrote: > Why not just call this max_pixel_count_step_up? Done.
lgtm
https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1999: cpu_adapted_ = false; On 2016/02/25 13:57:07, perkj_webrtc wrote: > On 2016/02/25 07:40:30, pthatcher1 wrote: > > It's still cpu_adapted sometimes, right? Just slightly less? Wouldn't we > have > > to clear both max_pixel_counts to be sure it's not adapted any more? > > Yes. This is just used for the info.adapt_reason stats. > I can count the number of ups / downs and that might be slightly better. The > problem might be screen share- that we don't adapt anyway.... Ie, say we adapt > down twice and then up once and then the screen share window is enlarged outside > our control and we don't get any more OnLoadUpdate. Are we then cpu constrained > or not? It seems tricky. Only the Source really knows if it constrained or not. But it doesn't know why (different sinks could have different reasons). The real question is: how does the source know when it's safe to remove the max_pixel_count_step_up? It would have to know that a given VideoFrame is one step up from that value set. Then it could clear it. Could it assume so as soon as it gets a frame that is larger than max_pixel_count_step_up? https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1478: // destroyed. Could we just implement WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate like so? void WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate(webrtc::LoadObserver::Load load) override { if (rtc::Thread::Current() != worker_thread_) { invoker_.AsyncInvoke<void>( worker_thread_, rtc::Bind(WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate, this, load)) return; } // ... current OnLoadUpdate ... } https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2002: ++number_of_cpu_adapt_changes_; If we track cpu_downgrades_ and cpu_upgrades_, would cpu_changes_ == cpu_downgrades_ + cpu_upgrades_? Would it make more sense to store it that way?
PTAL https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/280001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1999: cpu_adapted_ = false; On 2016/02/25 20:20:05, pthatcher1 wrote: > On 2016/02/25 13:57:07, perkj_webrtc wrote: > > On 2016/02/25 07:40:30, pthatcher1 wrote: > > > It's still cpu_adapted sometimes, right? Just slightly less? Wouldn't we > > have > > > to clear both max_pixel_counts to be sure it's not adapted any more? > > > > Yes. This is just used for the info.adapt_reason stats. > > I can count the number of ups / downs and that might be slightly better. The > > problem might be screen share- that we don't adapt anyway.... Ie, say we adapt > > down twice and then up once and then the screen share window is enlarged > outside > > our control and we don't get any more OnLoadUpdate. Are we then cpu > constrained > > or not? > > It seems tricky. Only the Source really knows if it constrained or not. But it > doesn't know why (different sinks could have different reasons). > > The real question is: how does the source know when it's safe to remove the > max_pixel_count_step_up? It would have to know that a given VideoFrame is one > step up from that value set. Then it could clear it. Could it assume so as > soon as it gets a frame that is larger than max_pixel_count_step_up? Can't the source just ignore max_pixel_value_count_step_up if it produce a resolution larger than this value? If the sink wanted a max it should set max_resolution_count. https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/base/videoa... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/base/videoa... webrtc/media/base/videoadapter.cc:505: *max_pixel_count_step_up <= GetOutputNumPixels()) { right- this will currently increase the resolution if a sink is added.... https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1478: // destroyed. On 2016/02/25 20:20:05, pthatcher1 wrote: > Could we just implement WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate > like so? > > > > void > WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate(webrtc::LoadObserver::Load > load) override { > if (rtc::Thread::Current() != worker_thread_) { > invoker_.AsyncInvoke<void>( > worker_thread_, > rtc::Bind(WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate, this, load)) > return; > } > > // ... current OnLoadUpdate ... > } // Invokes function objects (aka functors) asynchronously on a Thread, and // owns the lifetime of calls (ie, when this object is destroyed, calls in // flight are cancelled). AsyncInvoker can optionally execute a user-specified // function when the asynchronous call is complete, or operates in // fire-and-forget mode otherwise. I wonder why I was under the impression that messages in flight are not cancelled..... Of course we can. Thanks. https://codereview.webrtc.org/1695263002/diff/320001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2002: ++number_of_cpu_adapt_changes_; On 2016/02/25 20:20:05, pthatcher1 wrote: > If we track cpu_downgrades_ and cpu_upgrades_, would cpu_changes_ == > cpu_downgrades_ + cpu_upgrades_? Would it make more sense to store it that way? hum- I think the easiest is to move back the logic to not do cpu adaptation if this is screen cast for now. Then have separate counters- one that counts number of up / down in total and one that counts if we are currently restricted. This one we can reset when a capturer is disconnected. That is so that if we switch to sceen cast and back to a normal camera - it will not be messed up.
lgtm https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videoa... File webrtc/media/base/videoadapter.cc (right): https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videoa... webrtc/media/base/videoadapter.cc:509: OnCpuResolutionRequest(UPGRADE); Should we take this lock recursively or can you annotate OnCpuResolutionRequest? https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster_unittest.cc (right): https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster_unittest.cc:27: size_t number_of_rendered_frames_ = 0; I think this should be an int. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster_unittest.cc:69: TEST(VideoBroadcasterTest, wants_rotation_applied) { AppliesRotationIfAnySinkWantsRotationApplied or something https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster_unittest.cc:112: TEST(VideoBroadcasterTest, wants_max_pixel_count_step_up) { AppliesAMaxAboveMaxPixelCountStepUp https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1480: : thread_(rtc::Thread::Current()), worker_thread_ or main_thread_ maybe? https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1486: capturer_(NULL), nullptr https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1634: cpu_restricted_counter_ = 0; Comment on what this means. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1942: if (!capturer_ || capturer_is_screencast_) abort on !capturer_ before taking the lock. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1948: { remove {}s https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1952: if (!sink_wants_.max_pixel_count || Comment what this actually means. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1959: { remove {}s https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1963: if (sink_wants_.max_pixel_count || Same as above, comment please https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2008: if (capturer_ != NULL) { if (capturer_)
https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... File webrtc/media/base/videobroadcaster_unittest.cc (right): https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster_unittest.cc:27: size_t number_of_rendered_frames_ = 0; On 2016/02/26 14:26:18, pbos-webrtc wrote: > I think this should be an int. Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster_unittest.cc:91: TEST(VideoBroadcasterTest, wants_max_pixel_count) { AppliesMinOfSinkWantsMaxPixelCount https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/base/videob... webrtc/media/base/videobroadcaster_unittest.cc:112: TEST(VideoBroadcasterTest, wants_max_pixel_count_step_up) { On 2016/02/26 14:26:18, pbos-webrtc wrote: > AppliesAMaxAboveMaxPixelCountStepUp AppliesMinOfSinkWantsMaxPixelCountStepUP https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1480: : thread_(rtc::Thread::Current()), On 2016/02/26 14:26:18, pbos-webrtc wrote: > worker_thread_ or main_thread_ maybe? Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1486: capturer_(NULL), On 2016/02/26 14:26:18, pbos-webrtc wrote: > nullptr Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1634: cpu_restricted_counter_ = 0; On 2016/02/26 14:26:18, pbos-webrtc wrote: > Comment on what this means. Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1942: if (!capturer_ || capturer_is_screencast_) On 2016/02/26 14:26:18, pbos-webrtc wrote: > abort on !capturer_ before taking the lock. Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1948: { On 2016/02/26 14:26:18, pbos-webrtc wrote: > remove {}s Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1952: if (!sink_wants_.max_pixel_count || On 2016/02/26 14:26:18, pbos-webrtc wrote: > Comment what this actually means. Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1959: { On 2016/02/26 14:26:18, pbos-webrtc wrote: > remove {}s Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1963: if (sink_wants_.max_pixel_count || On 2016/02/26 14:26:18, pbos-webrtc wrote: > Same as above, comment please Done. https://codereview.webrtc.org/1695263002/diff/380001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2008: if (capturer_ != NULL) { On 2016/02/26 14:26:18, pbos-webrtc wrote: > if (capturer_) Done.
Patchset #20 (id:400001) has been deleted
lgtm https://codereview.webrtc.org/1695263002/diff/420001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/420001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1630: return false; {}s please
https://codereview.webrtc.org/1695263002/diff/420001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1695263002/diff/420001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1630: return false; On 2016/02/26 21:57:02, pthatcher1 wrote: > {}s please Done.
The CQ bit was checked by perkj@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, pbos@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1695263002/#ps460001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695263002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695263002/460001
Message was sent while issue was closed.
Description was changed from ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 ========== to ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 ========== to ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 Committed: https://crrev.com/2d5f0913f25a9e504eec2c2ecbb4e7647fd283b4 Cr-Commit-Position: refs/heads/master@{#11804} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/2d5f0913f25a9e504eec2c2ecbb4e7647fd283b4 Cr-Commit-Position: refs/heads/master@{#11804}
Message was sent while issue was closed.
Description was changed from ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Cl is WIP and uploaded to start the discussion. Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 Committed: https://crrev.com/2d5f0913f25a9e504eec2c2ecbb4e7647fd283b4 Cr-Commit-Position: refs/heads/master@{#11804} ========== to ========== Move direct use of VideoCapturer::VideoAdapter to VideoSinkWants. The purose of this cl is to remove dependency on cricket::VideoCapturer from WebRtcVideoChannel2. This cl change CPU adaptation to use a new VideoSinkWants.Resolution Tested on a N5 with hw acceleration turned off. BUG=webrtc:5426 Committed: https://crrev.com/2d5f0913f25a9e504eec2c2ecbb4e7647fd283b4 Cr-Commit-Position: refs/heads/master@{#11804} ========== |
