Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244213002:
Delete the class WebRtcVideoCapturerAdapter::MediaVideoFrameFactory (Closed)
|
Created:
4 years, 4 months ago by nisse-chromium (ooo August 14) Modified:
4 years, 4 months ago Reviewers:
perkj_chrome CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, Daniele Castagna Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelete the class WebRtcVideoCapturerAdapter::MediaVideoFrameFactory
Instead, use the AdaptFrame and OnFrame methods of the VideoCapturer
base class.
This cl also applies the new timestamp aligner logic, to translate
Chrome's camera timestamps into the timescale of rtc::TimeMicros(). No
other change in behavior is intended.
BUG=webrtc:5682, webrtc:5740, 516700
Committed: https://crrev.com/5b9a4b339447b033ca52244a02c0bef17067385f
Cr-Commit-Position: refs/heads/master@{#413093}
Patch Set 1 #Patch Set 2 : Use VideoSinkInterface in the unittests. Use natural_size as the pre-adaptation size. #
Total comments: 1
Patch Set 3 : Fix cropping. Delete two DCHECKs. #
Total comments: 2
Patch Set 4 : Keep visible_rect DCHECKs for now. #
Total comments: 2
Patch Set 5 : Addressed nits. #
Messages
Total messages: 36 (16 generated)
nisse@chromium.org changed reviewers: + perkj@chromium.org
The CQ bit was checked by nisse@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by perkj@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
On 2016/08/15 11:59:48, nisse-chromium wrote: I can reproduce the failure using out/gn-debug/content_unittests --gtest_filter=WebRtcVideoCapturerAdapterTest.CropFrameTo320320 I have to edit WebRtcVideoCapturerAdapterTest to use the VideoSinkInterface rather than SignalFrameCaptured.connect. But that's not quite enough, "natural_size" is lost. And I don't quite understand what that means. Current code sets orig_width and orig_height from input_frame->visible_rect(), and pass these on to AdaptFrame. Which doesn't modify the dimensions, since there's no video adapter. Should I instead use the "natural_size" as input to AdaptFrame?
https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:146: // Apply cropping parameters to the visible rectangle. This is most likely broken, crop parameters are in units of natural size, and furthermore natural_size may have a different aspect ratio than visible_rect. Does chrome ever use a VideoAdapter? If not, crop_x and crop_y are always zero and can be ignored. But we likely need some other cropping to preserve aspect ratio (ClampToCenteredSize in the old code).
On 2016/08/15 14:33:30, nisse-chromium wrote: > https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... > File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): > > https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:146: // Apply > cropping parameters to the visible rectangle. > This is most likely broken, crop parameters are in units of natural size, and > furthermore natural_size may have a different aspect ratio than visible_rect. > > Does chrome ever use a VideoAdapter? If not, crop_x and crop_y are always zero > and can be ignored. But we likely need some other cropping to preserve aspect > ratio (ClampToCenteredSize in the old code). From chrome you can do getusermedia with constraints. If you do that two times with different max and min width you will have a natural_size() != visible_rect. You can use the demo here to play around with it. https://webrtc.github.io/samples/src/content/peerconnection/constraints/ Tab 1: get media in 640*480 and connect. This will open the camera in 640*480 -> codec_size()=640*480 -> natural_size=640*480. Tab 2: get media in for example 500*480 and connect. The camera is still open in 640*480 -> codec_size()=640*480 -> natural_size=500*480.
On 2016/08/17 11:25:17, perkj_chrome wrote: > On 2016/08/15 14:33:30, nisse-chromium wrote: > > > https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... > > File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): > > > > > https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... > > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:146: // Apply > > cropping parameters to the visible rectangle. > > This is most likely broken, crop parameters are in units of natural size, and > > furthermore natural_size may have a different aspect ratio than visible_rect. > > > > Does chrome ever use a VideoAdapter? If not, crop_x and crop_y are always zero > > and can be ignored. But we likely need some other cropping to preserve aspect > > ratio (ClampToCenteredSize in the old code). > > From chrome you can do getusermedia with constraints. If you do that two times > with different max and min width you will have a natural_size() != visible_rect. > You can use the demo here to play around with it. > https://webrtc.github.io/samples/src/content/peerconnection/constraints/ > > Tab 1: get media in 640*480 and connect. This will open the camera in 640*480 -> > codec_size()=640*480 -> natural_size=640*480. > Tab 2: get media in for example 500*480 and connect. The camera is still open in > 640*480 -> codec_size()=640*480 -> natural_size=500*480. oh- if you your question is if cpu adaptation happens after this- then the answer is yes but that does not change aspect ratio.
On 2016/08/17 11:25:17, perkj_chrome wrote: > On 2016/08/15 14:33:30, nisse-chromium wrote: > > > https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... > > File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): > > > > > https://codereview.chromium.org/2244213002/diff/20001/content/renderer/media/... > > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:146: // Apply > > cropping parameters to the visible rectangle. > > This is most likely broken, crop parameters are in units of natural size, and > > furthermore natural_size may have a different aspect ratio than visible_rect. > > > > Does chrome ever use a VideoAdapter? If not, crop_x and crop_y are always zero > > and can be ignored. But we likely need some other cropping to preserve aspect > > ratio (ClampToCenteredSize in the old code). > > From chrome you can do getusermedia with constraints. If you do that two times > with different max and min width you will have a natural_size() != visible_rect. > You can use the demo here to play around with it. > https://webrtc.github.io/samples/src/content/peerconnection/constraints/ > > Tab 1: get media in 640*480 and connect. This will open the camera in 640*480 -> > codec_size()=640*480 -> natural_size=640*480. > Tab 2: get media in for example 500*480 and connect. The camera is still open in > 640*480 -> codec_size()=640*480 -> natural_size=500*480. Playing around with that gives me a crash, [1:14:0818/085612:FATAL:gpu_memory_buffer_video_frame_pool.cc(333)] Check failed: gfx::Rect(video_frame->coded_size()).Contains(gfx::Rect(output)). I think it's best you explain the media::VideoFrame dimensions face to face, before I make another attempt to get it right.
On 2016/08/17 11:46:12, perkj_chrome wrote: > oh- if you your question is if cpu adaptation happens after this- then the > answer is yes but that does not change aspect ratio. If I get this right, the VideoAdapter can change the aspect ratio if and only if the application calls OnOutputFormatRequest (thanks to magjed's changes a while back). In webrtc, the only calls to that are in android code, which isn't used in Chrome? But if not too complicated, it would be nice to get videoadapter cropping right too.
// Actual number of pixels in memory.const gfx <https://cs.chromium.org/chromium/src/ui/gfx/gpu_memory_buffer.h?l=27&ct=xref_... <https://cs.chromium.org/chromium/src/ui/gfx/geometry/size.h?l=25&ct=xref_jump... <https://cs.chromium.org/chromium/src/out/Debug/GENERATED/figments/cpp/LValueR...> coded_size <https://cs.chromium.org/chromium/src/media/base/video_frame.h?l=333&gs=cpp%25... const { return coded_size_ <https://cs.chromium.org/chromium/src/media/base/video_frame.h?l=526&ct=xref_j...>; } // The rect within coded_size that describes the actual pixels from memory to use. This rect is used when scaling to natural_size.const gfx <https://cs.chromium.org/chromium/src/ui/gfx/gpu_memory_buffer.h?l=27&ct=xref_... <https://cs.chromium.org/chromium/src/ui/gfx/geometry/rect.h?l=35&ct=xref_jump... <https://cs.chromium.org/chromium/src/out/Debug/GENERATED/figments/cpp/LValueR...> visible_rect <https://cs.chromium.org/chromium/src/media/base/video_frame.h?l=334&gs=cpp%25... const { return visible_rect_ <https://cs.chromium.org/chromium/src/media/base/video_frame.h?l=531&ct=xref_j...>; } // The requested size this frame should be rendered as. const gfx <https://cs.chromium.org/chromium/src/ui/gfx/gpu_memory_buffer.h?l=27&ct=xref_... <https://cs.chromium.org/chromium/src/ui/gfx/geometry/size.h?l=25&ct=xref_jump... <https://cs.chromium.org/chromium/src/out/Debug/GENERATED/figments/cpp/LValueR...> natural_size <https://cs.chromium.org/chromium/src/media/base/video_frame.h?l=335&gs=cpp%25... const { return natural_size_ <https://cs.chromium.org/chromium/src/media/base/video_frame.h?l=535&ct=xref_j...>; } On Thu, Aug 18, 2016 at 9:00 AM, <nisse@chromium.org> wrote: > On 2016/08/17 11:25:17, perkj_chrome wrote: > > On 2016/08/15 14:33:30, nisse-chromium wrote: > > > > > > https://codereview.chromium.org/2244213002/diff/20001/ > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc > > > File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc > (right): > > > > > > > > > https://codereview.chromium.org/2244213002/diff/20001/ > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc#newcode146 > > > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:146: > // Apply > > > cropping parameters to the visible rectangle. > > > This is most likely broken, crop parameters are in units of natural > size, > and > > > furthermore natural_size may have a different aspect ratio than > visible_rect. > > > > > > Does chrome ever use a VideoAdapter? If not, crop_x and crop_y are > always > zero > > > and can be ignored. But we likely need some other cropping to preserve > aspect > > > ratio (ClampToCenteredSize in the old code). > > > > From chrome you can do getusermedia with constraints. If you do that two > times > > with different max and min width you will have a natural_size() != > visible_rect. > > You can use the demo here to play around with it. > > https://webrtc.github.io/samples/src/content/peerconnection/constraints/ > > > > Tab 1: get media in 640*480 and connect. This will open the camera in > 640*480 > -> > > codec_size()=640*480 -> natural_size=640*480. > > Tab 2: get media in for example 500*480 and connect. The camera is still > open > in > > 640*480 -> codec_size()=640*480 -> natural_size=500*480. > > Playing around with that gives me a crash, > > [1:14:0818/085612:FATAL:gpu_memory_buffer_video_frame_pool.cc(333)] Check > failed: gfx::Rect(video_frame->coded_size()).Contains(gfx::Rect(output)). > > I think it's best you explain the media::VideoFrame dimensions face to > face, > before I make another attempt to get it right. > > https://codereview.chromium.org/2244213002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
There is no way to crop only the images going into a pc from js. ie, cropping, scaling etc is done done on the video track from js, not on the peerconnection to you dont need to care about that I think. The only think is CPU adaptation ==> down scaling. On Thu, Aug 18, 2016 at 9:06 AM, <nisse@chromium.org> wrote: > On 2016/08/17 11:46:12, perkj_chrome wrote: > > > oh- if you your question is if cpu adaptation happens after this- then > the > > answer is yes but that does not change aspect ratio. > > If I get this right, the VideoAdapter can change the aspect ratio if and > only if > the application calls OnOutputFormatRequest (thanks to magjed's changes a > while > back). In webrtc, the only calls to that are in android code, which isn't > used > in Chrome? > > But if not too complicated, it would be nice to get videoadapter cropping > right > too. > > > https://codereview.chromium.org/2244213002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/18 07:08:52, perkj_chrome wrote:
> // Actual number of pixels in memory.
> const gfx::Size& coded_size() const { return coded_size_; }
>
> // The rect within coded_size that describes the actual pixels from memory to
use. This rect is used when scaling to natural_size.
> const gfx::Rect& visible_rect() const { return visible_rect_; }
// The requested size this frame should be rendered as.
> const gfx::Size& natural_size() const { return natural_size_; }
And then it's the natural_size that should be input to video adaptation? And in
case there's no adaptation, the conversion to a webrtc video frame should create
a VideoFrameBuffer of size natural_size?
What about the cropping in the old code (ClampToCenteredSize)? It involves
visible_rect, natural_size (copied into the cricket::CapturedFrame in the
FrameFactory hack), and cropped_input_{width,height}, which are produced by the
video adapter.
As I understand you, natural_size and cropped_input_{width,height) will always
have the same aspect ratio, because the video adapter is only used for
cpu-related down scaling. But visible_rect may have a different aspect?
I've found a problem, which might be unrelated to what I'm trying to do.
I tried the constraint sliders, and ended up with a max height of 239 pixels.
Then the code in VideoTrackAdapter::VideoFrameResolutionAdapter::DeliverFrame
does some cropping (kind-of similar to what's done in
WebRtcVideoCapturerAdapter, it doesn't use ClampToCenteredSize though, but a
different function media::ComputeLetterboxRegion. There's some opportunity for
cleanup). The input is a visible_rect of
{origin_ = {x_ = 0, y_ = 0}, size_ = {width_ = 480, height_ = 270}}
and desired_size
{width_ = 480, height_ = 239}.
It computes the cropped region_in_frame
{origin_ = {x_ = 0, y_ = 15}, size_ = {width_ = 480, height_ = 239}}
and constructs a new frame as
video_frame = media::VideoFrame::WrapVideoFrame(
frame, frame->format(), region_in_frame, desired_size);
Later, after a couple of thread jumps and task queues, this frame ends up in
media::GpuMemoryBufferVideoFramePool::PoolImpl::CreateHardwareFrame, where it
fails an assert,
DCHECK((video_frame->visible_rect().y() & 1) == 0);
So this code refuses to handle an I420 frame where visible_rect.x() or .y() are
odd. It does tolerate odd width and height, though, those are simply rounded up
to make them even, which should be safe if and only if coded_size for I420
always are even.
I'll try to round .x and .y down to even, and see what happens.
nisse@chromium.org changed reviewers: + dcastagna@chromium.org
Seems to work now, I added workarounds for http://crbug/638906. Per, could you give it another cq dry run? dcastagna, are any further changes needed to handle odd visible_rect().x() and .y() correctly?
The CQ bit was checked by perkj@chromium.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.chromium.or...
On 2016/08/18 12:51:20, nisse-chromium wrote: > dcastagna, are any further changes needed to handle odd visible_rect().x() and > .y() correctly? I've put back the DCHECKs for now, the problems described in http://crbug/638906 are not triggered by the testsuite (but can still be triggered by setting unusual constraints from js, independently of this change).
nisse@chromium.org changed reviewers: - dcastagna@chromium.org
Dropped the DCHECK change. Ok now?
lgtm with the below addressed. https://codereview.chromium.org/2244213002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2244213002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:177: // We need to scale the frame before we hand it over to cricket. please remove the word cricket. It is a ns that is beeing deprecated. https://codereview.chromium.org/2244213002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2244213002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:135: adapted_width &= ~1; please remove this as well for now since it does not completely fix the problem.
https://codereview.chromium.org/2244213002/diff/40001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2244213002/diff/40001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:177: // We need to scale the frame before we hand it over to cricket. On 2016/08/19 08:33:20, perkj_chrome wrote: > please remove the word cricket. It is a ns that is beeing deprecated. I guess you mean 'replace by "webrtc"' rather than 'remove'? Done. https://codereview.chromium.org/2244213002/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2244213002/diff/60001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:135: adapted_width &= ~1; On 2016/08/19 08:33:20, perkj_chrome wrote: > please remove this as well for now since it does not completely fix the problem. Done.
The CQ bit was checked by nisse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org Link to the patchset: https://codereview.chromium.org/2244213002/#ps80001 (title: "Addressed nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Delete the class WebRtcVideoCapturerAdapter::MediaVideoFrameFactory Instead, use the AdaptFrame and OnFrame methods of the VideoCapturer base class. This cl also applies the new timestamp aligner logic, to translate Chrome's camera timestamps into the timescale of rtc::TimeMicros(). No other change in behavior is intended. BUG=webrtc:5682, webrtc:5740, 516700 ========== to ========== Delete the class WebRtcVideoCapturerAdapter::MediaVideoFrameFactory Instead, use the AdaptFrame and OnFrame methods of the VideoCapturer base class. This cl also applies the new timestamp aligner logic, to translate Chrome's camera timestamps into the timescale of rtc::TimeMicros(). No other change in behavior is intended. BUG=webrtc:5682, webrtc:5740, 516700 Committed: https://crrev.com/5b9a4b339447b033ca52244a02c0bef17067385f Cr-Commit-Position: refs/heads/master@{#413093} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5b9a4b339447b033ca52244a02c0bef17067385f Cr-Commit-Position: refs/heads/master@{#413093}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2263183002/ by nisse@chromium.org. The reason for reverting is: Somehow breaks timestamping. Reverting for investigation. To reproduce, run ./browser_tests --gtest_filter='WebRtcVideoQualityBrowserTests/WebRtcVideoQualityBrowserTest*' --run-manual --ui-test-action-max-timeout=200000 This produces lots of warnings like [1:14:0822/153830:WARNING:video_capture_input.cc(79)] Same/old NTP timestamp (3680861906110 <= 3680861906110) for incoming frame. Dropping. [1:14:0822/153830:WARNING:video_capture_input.cc(79)] Same/old NTP timestamp (3680861906042 <= 3680861906042) for incoming frame. Dropping. . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
