Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182183007:
Handle scaling frames in RTCVideoEncoder (Closed)
|
Created:
4 years, 4 months ago by emircan Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle scaling frames in RTCVideoEncoder
This CL adds support for scaling incoming video frames in RTCVideoEncoder.
Earlier, we expected WebRTC to only send frames that have the same size
as the initialized size. However, after last changes we can have different
sizes.
I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still
does only a copy when the gven input and output sizes are the same:
https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0&l=1400
BUG=630577
TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest.
Committed: https://crrev.com/35ad929a8daded62262cdd91018dc50350483bb3
Cr-Commit-Position: refs/heads/master@{#409286}
Patch Set 1 #Patch Set 2 : Add unittests. #
Total comments: 28
Patch Set 3 : Comments. #Patch Set 4 : Add magjed TODO. #Patch Set 5 : posciak@ comments. #
Messages
Total messages: 59 (43 generated)
The CQ bit was checked by emircan@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...
Description was changed from ========== adding scale BUG= ========== to ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. ==========
emircan@chromium.org changed reviewers: + tommi@chromium.org, wuchengli@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emircan@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...
Description was changed from ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. ========== to ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittests. ==========
emircan@chromium.org changed reviewers: + magjed@chromium.org
PTAL.
Description was changed from ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittests. ========== to ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. ==========
Description was changed from ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. ========== to ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does only a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by emircan@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by emircan@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...
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:607: libyuv::kFilterBilinear)) { Do we have an idea of the performance impact of bilinear filtering? I'm wondering if we could perhaps roughly measure it with the unittest by doing a copy and comparing to scaling? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_unittest.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:6: #include <string.h> Is string.h needed? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:82: codec.startBitrate = 100; Could this be a constant defined together with the size? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:91: client->RequireBitstreamBuffers(0, input_visible_size, 12345); Perhaps we could use input_visible_size_.GetArea() for the last argument (as a rough estimation)? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:103: void VerifyEncodeFrame(const scoped_refptr<media::VideoFrame>& frame, Perhaps s/Encode/Encoded/ ? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:132: rtc_encoder_->InitEncode(&codec, 1, 12345); Should we check return values from InitEncode() and Encode()? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:139: webrtc::I420Buffer::Create(kInputFrameWidth, kInputFrameHeight); Would it be possible to prefill buffers with one color, say red for input, and black for output, and verify output has scaled red as a result? https://codereview.chromium.org/2182183007/diff/60001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/2182183007/diff/60001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.cc:105: shared_memory->CreateAndMapAnonymous(size); Should we check return value?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:607: libyuv::kFilterBilinear)) { On 2016/07/28 01:48:08, Pawel Osciak wrote: > Do we have an idea of the performance impact of bilinear filtering? I'm > wondering if we could perhaps roughly measure it with the unittest by doing a > copy and comparing to scaling? +1 https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_unittest.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:77: webrtc::VideoCodec codec; skip memset and just do: webrtc::VideoCodec codec = {}; https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:113: base::Thread vea_thread_; encoder_thread_? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:116: scoped_refptr<base::SingleThreadTaskRunner> vea_task_runner_; needed? or is vea_thread_.task_runner() enough?
https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:607: libyuv::kFilterBilinear)) { On 2016/07/28 01:48:08, Pawel Osciak wrote: > Do we have an idea of the performance impact of bilinear filtering? I'm > wondering if we could perhaps roughly measure it with the unittest by doing a > copy and comparing to scaling? I think we should use libyuv::kFilterBox instead of libyuv::kFilterBilinear, to avoid aliasing when downsampling. libyuv::kFilterBilinear means bilinear point sampling and is faster than proper box scaling, but the performance difference is negligible for optimized 1/2 and 1/4 scaling. Scaling from 1280x720 to 176x144 is not optimized though. The optimal solution in terms of quality and performance is to do an image pyramid with box scaling 1280x720 -> 640x360 -> 320x180 -> 176x144, instead of 1280x720 -> 640x360, 1280x720 -> 320x180, and 1280x720 -> 176x144, but I'm not sure how to implement that since the RTCVideoEncoders are unaware of each other. SimulcastEncoderAdapter uses libyuv::kFilterBilinear, so using it here as well would not be a quality regression. I did a quick benchmark of libyuv scaling (each operation is repeated 1000 times, so cache hit is optimal, and these numbers might be too low compared to real use). Using Neon (on a Nexus 5): Bilinear Box 1280x720->176x144: 0.21ms, 4.51ms 1280x720->320x180: 0.48ms, 0.40ms 1280x720->640x360: 0.35ms, 0.33ms 1280x720 copy: 1.14ms x86 (AVX) (on my workstation): Bilinear Box 1280x720->176x144: 0.07ms, 1.04ms 1280x720->320x180: 0.14ms, 0.06ms 1280x720->640x360: 0.09ms, 0.09ms 1280x720 copy: 0.13ms So 1280x720->176x144 with box scaling is ridiculously slow, it's actually faster to do 1280x720->640x360->320x180->176x144. I will put it on my todo list to fix that in libyuv.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by emircan@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...
https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:607: libyuv::kFilterBilinear)) { On 2016/07/28 01:48:08, Pawel Osciak wrote: > Do we have an idea of the performance impact of bilinear filtering? I'm > wondering if we could perhaps roughly measure it with the unittest by doing a > copy and comparing to scaling? SimulcastEncoderAdapter uses libyuv::kFilterBilinear, so there wouldn't be a quality or performance difference if there is scaling in comparison to before. If input and output sizes are the same, it falls back to just copying the frame[0]. It will be the same codepath as libyuv::I420Copy before[1] hence the same performance. I tried to note that in the CL description as well. [0] https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... [1] https://cs.chromium.org/chromium/src/third_party/libyuv/source/convert.cc?rcl... https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:607: libyuv::kFilterBilinear)) { On 2016/07/28 10:27:19, magjed_chromium wrote: > On 2016/07/28 01:48:08, Pawel Osciak wrote: > > Do we have an idea of the performance impact of bilinear filtering? I'm > > wondering if we could perhaps roughly measure it with the unittest by doing a > > copy and comparing to scaling? > > I think we should use libyuv::kFilterBox instead of libyuv::kFilterBilinear, to > avoid aliasing when downsampling. libyuv::kFilterBilinear means bilinear point > sampling and is faster than proper box scaling, but the performance difference > is negligible for optimized 1/2 and 1/4 scaling. Scaling from 1280x720 to > 176x144 is not optimized though. The optimal solution in terms of quality and > performance is to do an image pyramid with box scaling 1280x720 -> 640x360 -> > 320x180 -> 176x144, instead of 1280x720 -> 640x360, 1280x720 -> 320x180, and > 1280x720 -> 176x144, but I'm not sure how to implement that since the > RTCVideoEncoders are unaware of each other. > > SimulcastEncoderAdapter uses libyuv::kFilterBilinear, so using it here as well > would not be a quality regression. > > I did a quick benchmark of libyuv scaling (each operation is repeated 1000 > times, so cache hit is optimal, and these numbers might be too low compared to > real use). > Using Neon (on a Nexus 5): > Bilinear Box > 1280x720->176x144: 0.21ms, 4.51ms > 1280x720->320x180: 0.48ms, 0.40ms > 1280x720->640x360: 0.35ms, 0.33ms > 1280x720 copy: 1.14ms > > x86 (AVX) (on my workstation): > Bilinear Box > 1280x720->176x144: 0.07ms, 1.04ms > 1280x720->320x180: 0.14ms, 0.06ms > 1280x720->640x360: 0.09ms, 0.09ms > 1280x720 copy: 0.13ms > > So 1280x720->176x144 with box scaling is ridiculously slow, it's actually faster > to do 1280x720->640x360->320x180->176x144. I will put it on my todo list to fix > that in libyuv. Thanks for the metrics. Would you recommend changing to libyuv::kFilterBox right now despite the performance? Or we can add a TODO here to followup after the improvements libyuv::kFilterBox rolls in? https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_unittest.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:6: #include <string.h> On 2016/07/28 01:48:08, Pawel Osciak wrote: > Is string.h needed? Removed. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:77: webrtc::VideoCodec codec; On 2016/07/28 07:40:40, tommi-chrömium wrote: > skip memset and just do: > webrtc::VideoCodec codec = {}; Done. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:82: codec.startBitrate = 100; On 2016/07/28 01:48:08, Pawel Osciak wrote: > Could this be a constant defined together with the size? Done. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:91: client->RequireBitstreamBuffers(0, input_visible_size, 12345); On 2016/07/28 01:48:08, Pawel Osciak wrote: > Perhaps we could use input_visible_size_.GetArea() for the last argument (as a > rough estimation)? Done. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:103: void VerifyEncodeFrame(const scoped_refptr<media::VideoFrame>& frame, On 2016/07/28 01:48:08, Pawel Osciak wrote: > Perhaps s/Encode/Encoded/ ? Done. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:113: base::Thread vea_thread_; On 2016/07/28 07:40:40, tommi-chrömium wrote: > encoder_thread_? Done. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:116: scoped_refptr<base::SingleThreadTaskRunner> vea_task_runner_; On 2016/07/28 07:40:40, tommi-chrömium wrote: > needed? or is vea_thread_.task_runner() enough? Done. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:132: rtc_encoder_->InitEncode(&codec, 1, 12345); On 2016/07/28 01:48:08, Pawel Osciak wrote: > Should we check return values from InitEncode() and Encode()? I was checking it in the previous test, but it definitely would help to check again. I'm adding both. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:139: webrtc::I420Buffer::Create(kInputFrameWidth, kInputFrameHeight); On 2016/07/28 01:48:08, Pawel Osciak wrote: > Would it be possible to prefill buffers with one color, say red for input, and > black for output, and verify output has scaled red as a result? I prefill the buffers with black and verify it in the result now. I only check for planes' first pixels, so the test doesn't take too long, but can change if you wish. I couldn't understand what you mean by red&black though. Do you suggest filling the buffers with a pattern? https://codereview.chromium.org/2182183007/diff/60001/media/renderers/mock_gp... File media/renderers/mock_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/2182183007/diff/60001/media/renderers/mock_gp... media/renderers/mock_gpu_video_accelerator_factories.cc:105: shared_memory->CreateAndMapAnonymous(size); On 2016/07/28 01:48:08, Pawel Osciak wrote: > Should we check return value? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:607: libyuv::kFilterBilinear)) { On 2016/07/28 19:35:45, emircan wrote: > On 2016/07/28 10:27:19, magjed_chromium wrote: > > On 2016/07/28 01:48:08, Pawel Osciak wrote: > > > Do we have an idea of the performance impact of bilinear filtering? I'm > > > wondering if we could perhaps roughly measure it with the unittest by doing > a > > > copy and comparing to scaling? > > > > I think we should use libyuv::kFilterBox instead of libyuv::kFilterBilinear, > to > > avoid aliasing when downsampling. libyuv::kFilterBilinear means bilinear point > > sampling and is faster than proper box scaling, but the performance difference > > is negligible for optimized 1/2 and 1/4 scaling. Scaling from 1280x720 to > > 176x144 is not optimized though. The optimal solution in terms of quality and > > performance is to do an image pyramid with box scaling 1280x720 -> 640x360 -> > > 320x180 -> 176x144, instead of 1280x720 -> 640x360, 1280x720 -> 320x180, and > > 1280x720 -> 176x144, but I'm not sure how to implement that since the > > RTCVideoEncoders are unaware of each other. > > > > SimulcastEncoderAdapter uses libyuv::kFilterBilinear, so using it here as well > > would not be a quality regression. > > > > I did a quick benchmark of libyuv scaling (each operation is repeated 1000 > > times, so cache hit is optimal, and these numbers might be too low compared to > > real use). > > Using Neon (on a Nexus 5): > > Bilinear Box > > 1280x720->176x144: 0.21ms, 4.51ms > > 1280x720->320x180: 0.48ms, 0.40ms > > 1280x720->640x360: 0.35ms, 0.33ms > > 1280x720 copy: 1.14ms > > > > x86 (AVX) (on my workstation): > > Bilinear Box > > 1280x720->176x144: 0.07ms, 1.04ms > > 1280x720->320x180: 0.14ms, 0.06ms > > 1280x720->640x360: 0.09ms, 0.09ms > > 1280x720 copy: 0.13ms > > > > So 1280x720->176x144 with box scaling is ridiculously slow, it's actually > faster > > to do 1280x720->640x360->320x180->176x144. I will put it on my todo list to > fix > > that in libyuv. > > Thanks for the metrics. Would you recommend changing to libyuv::kFilterBox right > now despite the performance? Or we can add a TODO here to followup after the > improvements libyuv::kFilterBox rolls in? I think it's ok to use kFilterBilinear for now. You can put the TODO on me, something like: TODO(magjed): Downscale with kFilterBox in an image pyramid instead.
emircan@chromium.org changed reviewers: + dalecurtis@chromium.org
Thanks. I am adding dalecurtis@ for OWNERS review on /media/*. PTAL.
lgtm
The CQ bit was checked by emircan@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...
Thanks. posciak@/wuchengli@ can you PTAL as content/renderer/media/gpu/* OWNERS?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/08/01 23:31:29, emircan wrote: > Thanks. > > posciak@/wuchengli@ can you PTAL as content/renderer/media/gpu/* OWNERS? Pawel will review this one.
lgtm % nit https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_unittest.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:139: webrtc::I420Buffer::Create(kInputFrameWidth, kInputFrameHeight); On 2016/07/28 19:35:45, emircan wrote: > On 2016/07/28 01:48:08, Pawel Osciak wrote: > > Would it be possible to prefill buffers with one color, say red for input, and > > black for output, and verify output has scaled red as a result? > > I prefill the buffers with black and verify it in the result now. I only check > for planes' first pixels, so the test doesn't take too long, but can change if > you wish. > I couldn't understand what you mean by red&black though. Do you suggest filling > the buffers with a pattern? Patterns would be preferable, we could maybe have a checkerboard and see if it was scaled correctly. But I agree that may be too much for this test, I'll leave it up to you. If we keep the solid color, I'd suggest perhaps something different than black?
lgtm
The CQ bit was checked by emircan@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...
Thanks. https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder_unittest.cc (right): https://codereview.chromium.org/2182183007/diff/60001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder_unittest.cc:139: webrtc::I420Buffer::Create(kInputFrameWidth, kInputFrameHeight); On 2016/08/02 05:53:20, Pawel Osciak wrote: > On 2016/07/28 19:35:45, emircan wrote: > > On 2016/07/28 01:48:08, Pawel Osciak wrote: > > > Would it be possible to prefill buffers with one color, say red for input, > and > > > black for output, and verify output has scaled red as a result? > > > > I prefill the buffers with black and verify it in the result now. I only > check > > for planes' first pixels, so the test doesn't take too long, but can change if > > you wish. > > I couldn't understand what you mean by red&black though. Do you suggest > filling > > the buffers with a pattern? > > Patterns would be preferable, we could maybe have a checkerboard and see if it > was scaled correctly. But I agree that may be too much for this test, I'll leave > it up to you. If we keep the solid color, I'd suggest perhaps something > different than black? I changed the color to a custom one. I will refrain from a more complicated pattern test for now, as it looks like re-testing libyuv::I420Scale. We can add it when we change filter and change downscaling as described in TODO.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
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: This issue passed the CQ dry run.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, magjed@chromium.org, tommi@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2182183007/#ps140001 (title: "posciak@ comments.")
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.
Description was changed from ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does only a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. ========== to ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does only a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does only a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. ========== to ========== Handle scaling frames in RTCVideoEncoder This CL adds support for scaling incoming video frames in RTCVideoEncoder. Earlier, we expected WebRTC to only send frames that have the same size as the initialized size. However, after last changes we can have different sizes. I replaced libyuv::I420Copy with libyuv::I420Scale, but note that it still does only a copy when the gven input and output sizes are the same: https://cs.chromium.org/chromium/src/third_party/libyuv/source/scale.cc?rcl=0... BUG=630577 TEST=Tested Hangouts call on veyron_jerry. Added RTCVideoEncoderUnittest. Committed: https://crrev.com/35ad929a8daded62262cdd91018dc50350483bb3 Cr-Commit-Position: refs/heads/master@{#409286} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/35ad929a8daded62262cdd91018dc50350483bb3 Cr-Commit-Position: refs/heads/master@{#409286} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
