Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(777)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1306693002: media: Convert I420 VideoFrame to UYVY GpuMemoryBuffer. (Closed)

Created:
5 years, 4 months ago by Daniele Castagna
Modified:
5 years, 4 months ago
CC:
Aaron Boodman, abarth-chromium, asvitkine+watch_chromium.org, ben+mojo_chromium.org, cc-bugs_chromium.org, ccameron, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, davemoore+watch_chromium.org, dzhioev+watch_chromium.org, feature-media-reviews_chromium.org, jam, jbauman+watch_chromium.org, kalyank, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, oshima+watch_chromium.org, ozone-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, sievers+watch_chromium.org, viettrungluu+watch_chromium.org, wjia+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

media: Convert I420 VideoFrame to UYVY GpuMemoryBuffer. This patch adds support for UYVY as a pixel format for the VideoFrame backed by GpuMemoryBuffer. If UYVY is set as format in GpuMemoryBufferVideoFramePool, the source VideoFrame will be converted during the copy to GMB. UYVY VideoFrames might get promoted to overlays on Mac. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel BUG=485859, 510252 Committed: https://crrev.com/de00d08970e7ef2a95b231d605bb6a0c2c7716f4 Cr-Commit-Position: refs/heads/master@{#344899}

Patch Set 1 #

Patch Set 2 : Cleaned up. VideoFormat propagated. Test. #

Patch Set 3 : Cleaned up. VideoFormat propagated. Test. #

Total comments: 14

Patch Set 4 : Rebase. Address andresantoso's comments. #

Total comments: 29

Patch Set 5 : Address reviewers' comments. #

Total comments: 2

Patch Set 6 : s/CHECK_EQ/DCHECK_EQ. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+229 lines, -72 lines) Patch
M content/renderer/media/renderer_gpu_video_accelerator_factories.h View 1 2 3 4 4 chunks +7 lines, -1 line 0 comments Download
M content/renderer/media/renderer_gpu_video_accelerator_factories.cc View 1 2 3 4 4 chunks +9 lines, -1 line 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 1 chunk +6 lines, -1 line 0 comments Download
M media/base/video_frame.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M media/blink/skcanvas_video_renderer.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M media/renderers/gpu_video_accelerator_factories.h View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
M media/renderers/mock_gpu_video_accelerator_factories.h View 1 2 3 4 2 chunks +11 lines, -0 lines 0 comments Download
M media/renderers/mock_gpu_video_accelerator_factories.cc View 1 2 3 4 chunks +14 lines, -5 lines 0 comments Download
M media/video/gpu_memory_buffer_video_frame_pool.cc View 1 2 3 4 5 15 chunks +152 lines, -61 lines 0 comments Download
M media/video/gpu_memory_buffer_video_frame_pool_unittest.cc View 1 2 3 4 1 chunk +22 lines, -0 lines 0 comments Download

Messages

Total messages: 30 (12 generated)
Daniele Castagna
PTAL.
5 years, 4 months ago (2015-08-21 13:57:05 UTC) #5
Daniele Castagna
5 years, 4 months ago (2015-08-21 17:38:14 UTC) #7
Andre
https://codereview.chromium.org/1306693002/diff/100001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1306693002/diff/100001/media/base/video_frame.cc#newcode227 media/base/video_frame.cc:227: DLOG(ERROR) << "Only ARGB pixel format supported, got " ...
5 years, 4 months ago (2015-08-21 18:18:36 UTC) #8
Daniele Castagna
https://codereview.chromium.org/1306693002/diff/100001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1306693002/diff/100001/media/base/video_frame.cc#newcode227 media/base/video_frame.cc:227: DLOG(ERROR) << "Only ARGB pixel format supported, got " ...
5 years, 4 months ago (2015-08-21 18:49:02 UTC) #9
Andre
LGTM
5 years, 4 months ago (2015-08-21 20:06:17 UTC) #10
Daniele Castagna
+avi for content/
5 years, 4 months ago (2015-08-21 20:16:25 UTC) #12
Daniele Castagna
+xhwang for media/
5 years, 4 months ago (2015-08-21 20:27:18 UTC) #14
xhwang
lgtm. I only have a few nits. https://codereview.chromium.org/1306693002/diff/110001/content/renderer/media/renderer_gpu_video_accelerator_factories.h File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://codereview.chromium.org/1306693002/diff/110001/content/renderer/media/renderer_gpu_video_accelerator_factories.h#newcode79 content/renderer/media/renderer_gpu_video_accelerator_factories.h:79: media::VideoPixelFormat VideoFramePixelFormat() ...
5 years, 4 months ago (2015-08-21 20:53:07 UTC) #15
Avi (use Gerrit)
https://codereview.chromium.org/1306693002/diff/110001/media/video/gpu_memory_buffer_video_frame_pool.cc File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1306693002/diff/110001/media/video/gpu_memory_buffer_video_frame_pool.cc#newcode332 media/video/gpu_memory_buffer_video_frame_pool.cc:332: CHECK(frame_resources->plane_resources[i].gpu_memory_buffer->Map(&data)); Why CHECK and not DCHECK?
5 years, 4 months ago (2015-08-21 20:57:47 UTC) #16
Daniele Castagna
https://codereview.chromium.org/1306693002/diff/110001/content/renderer/media/renderer_gpu_video_accelerator_factories.h File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://codereview.chromium.org/1306693002/diff/110001/content/renderer/media/renderer_gpu_video_accelerator_factories.h#newcode79 content/renderer/media/renderer_gpu_video_accelerator_factories.h:79: media::VideoPixelFormat VideoFramePixelFormat() override; On 2015/08/21 at 20:53:07, xhwang wrote: ...
5 years, 4 months ago (2015-08-21 21:28:18 UTC) #19
Avi (use Gerrit)
LGTM with DCHECK change. https://codereview.chromium.org/1306693002/diff/170001/media/video/gpu_memory_buffer_video_frame_pool.cc File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1306693002/diff/170001/media/video/gpu_memory_buffer_video_frame_pool.cc#newcode331 media/video/gpu_memory_buffer_video_frame_pool.cc:331: GpuMemoryBufferFormat(output_format_, i))); DCHECK here too
5 years, 4 months ago (2015-08-21 21:32:04 UTC) #20
Daniele Castagna
https://codereview.chromium.org/1306693002/diff/170001/media/video/gpu_memory_buffer_video_frame_pool.cc File media/video/gpu_memory_buffer_video_frame_pool.cc (right): https://codereview.chromium.org/1306693002/diff/170001/media/video/gpu_memory_buffer_video_frame_pool.cc#newcode331 media/video/gpu_memory_buffer_video_frame_pool.cc:331: GpuMemoryBufferFormat(output_format_, i))); On 2015/08/21 at 21:32:03, Avi wrote: > ...
5 years, 4 months ago (2015-08-21 21:37:08 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306693002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306693002/190001
5 years, 4 months ago (2015-08-21 21:42:23 UTC) #24
reveman
https://codereview.chromium.org/1306693002/diff/110001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1306693002/diff/110001/content/renderer/render_thread_impl.cc#newcode1359 content/renderer/render_thread_impl.cc:1359: cmd_line->GetSwitchValueASCII(switches::kVideoImageTextureTarget); How can we know what the correct image ...
5 years, 4 months ago (2015-08-21 21:53:03 UTC) #26
Daniele Castagna
https://codereview.chromium.org/1306693002/diff/110001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1306693002/diff/110001/content/renderer/render_thread_impl.cc#newcode1359 content/renderer/render_thread_impl.cc:1359: cmd_line->GetSwitchValueASCII(switches::kVideoImageTextureTarget); On 2015/08/21 at 21:53:02, reveman wrote: > How ...
5 years, 4 months ago (2015-08-21 22:46:04 UTC) #27
commit-bot: I haz the power
Committed patchset #6 (id:190001)
5 years, 4 months ago (2015-08-21 23:18:20 UTC) #28
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/de00d08970e7ef2a95b231d605bb6a0c2c7716f4 Cr-Commit-Position: refs/heads/master@{#344899}
5 years, 4 months ago (2015-08-21 23:19:14 UTC) #29
reveman
5 years, 4 months ago (2015-08-22 14:04:46 UTC) #30
Message was sent while issue was closed. 
https://codereview.chromium.org/1306693002/diff/110001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1306693002/diff/110001/media/base/video_frame...
media/base/video_frame.cc:226: if (format != PIXEL_FORMAT_ARGB && format !=
PIXEL_FORMAT_UYVY) {
On 2015/08/21 at 22:46:04, Daniele Castagna wrote:
> On 2015/08/21 at 21:53:03, reveman wrote:
> > why does this change?
> 
> We need to send a VideoFrame that contains one mailbox with one native texture
where the format of the VideoFrame is UYUV.

don't we sample out of this texture as if it was ARGB and the driver does the
color space conversion? just like other native textures produced by hardware
decode..

https://codereview.chromium.org/1306693002/diff/110001/media/blink/skcanvas_v...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1306693002/diff/110001/media/blink/skcanvas_v...
media/blink/skcanvas_video_renderer.cc:142: PIXEL_FORMAT_UYVY ==
video_frame->format());
On 2015/08/21 at 22:46:04, Daniele Castagna wrote:
> On 2015/08/21 at 21:53:03, reveman wrote:
> > and why this change?
> 
> Same as the above, if the VideoFrame gets converted to GMBs, it can end up in
PIXEL_FORMAT_UYVY format.

what is this format used for? determines how to sample from the native textures
or something else?

https://codereview.chromium.org/1306693002/diff/110001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1306693002/diff/110001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:145: const VideoPixelFormat
output_format_;
On 2015/08/21 at 22:46:04, Daniele Castagna wrote:
> On 2015/08/21 at 21:53:03, reveman wrote:
> > why is output_format_ of type VideoPixelFormat? what will this value be on
ChromeOS in the future when we start using BufferFormat::YUV_420 as output
format?
> 
> That is what we tried to address long time ago with crrev.com/1117423002 when
we introduced TextureFormat.
> The change has been basically removed and we'll have a problem once we want to
do what you're describing.

We should have fixed that before landing (or as part of) this change as this
makes it incomplete and confusing.

https://codereview.chromium.org/1306693002/diff/110001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:332:
CHECK(frame_resources->plane_resources[i].gpu_memory_buffer->Map(&data));
On 2015/08/21 at 21:28:18, Daniele Castagna wrote:
> On 2015/08/21 at 20:57:46, Avi wrote:
> > Why CHECK and not DCHECK?
> 
> I remember I had a discussion with reveman@ about this and he convinced me it
was a good idea. Right now I don't remember what the rationale was though. :/
> 
> A quick search shows there is a DCHECK in all the other places where map is
called.
> Changing it to DCHECK for consistency.

This can typically only fail when OOM.

Use a DCHECK if you're able to handle this situation in a way that doesn't
result in a confusing crash. If failure to map the buffer is going to result in
a confusing crash which is hard to diagnose then use a CHECK here instead.

Powered by Google App Engine
This is Rietveld 408576698