Chromium Code Reviews
Help | Chromium Project | Sign in
(1012)

Issue 11274017: Added support for YUV videos to the software compositor. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by slavi
Modified:
1 year, 5 months ago
CC:
chromium-reviews_chromium.org, feature-media-reviews_chromium.org, cc-bugs_chromium.org, darin-cc_chromium.org, piman
Visibility:
Public.

Description

Added support for YUV videos to the software compositor.

Those Layout Tests pass with this patch (minus filtering differences):

platform/chromium/virtual/softwarecompositing/geometry/video-fixed-scrolling.html
platform/chromium/virtual/softwarecompositing/geometry/video-opacity-overlay.html
platform/chromium/virtual/softwarecompositing/layers-inside-overflow-scroll.html
platform/chromium/virtual/softwarecompositing/overflow/scroll-ancestor-update.html
platform/chromium/virtual/softwarecompositing/self-painting-layers.html
platform/chromium/virtual/softwarecompositing/reflections/load-video-in-reflection.html
platform/chromium/virtual/softwarecompositing/video-page-visibility.html
platform/chromium/virtual/softwarecompositing/visibility/visibility-simple-video-layer.html



BUG=150016
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164871

Patch Set 1 #

Patch Set 2 : Cosmetic. #

Patch Set 3 : More cosmetics. #

Total comments: 2

Patch Set 4 : Fixed a logic bug. #

Patch Set 5 : Fixed shared lib builds. #

Patch Set 6 : Moved YUV conversion to video_layer_impl. #

Patch Set 7 : Some nits. #

Total comments: 4

Patch Set 8 : Rebase and addressing review comment. #

Patch Set 9 : Addressing code review. #

Total comments: 2

Patch Set 10 : Rebase. #

Patch Set 11 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+233 lines, -119 lines) Lint Patch
M cc/DEPS View 1 chunk +1 line, -0 lines 0 comments ? errors Download
M cc/cc.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments ? errors Download
M cc/cc_tests.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments ? errors Download
M cc/gl_renderer.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -1 line 0 comments ? errors Download
M cc/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 chunks +51 lines, -22 lines 0 comments ? errors Download
M cc/software_renderer.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -1 line 0 comments ? errors Download
M cc/software_renderer.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -3 lines 0 comments 3 errors Download
M cc/video_layer.h View 1 2 2 chunks +14 lines, -3 lines 0 comments ? errors Download
M cc/video_layer.cc View 1 2 2 chunks +10 lines, -7 lines 0 comments 2 errors Download
M cc/video_layer_impl.h View 1 2 3 4 5 6 7 5 chunks +21 lines, -8 lines 0 comments ? errors Download
M cc/video_layer_impl.cc View 1 2 3 4 5 6 12 chunks +107 lines, -61 lines 0 comments ? errors Download
M webkit/compositor_bindings/web_video_layer_impl.h View 1 chunk +0 lines, -1 line 0 comments ? errors Download
M webkit/compositor_bindings/web_video_layer_impl.cc View 1 2 2 chunks +8 lines, -3 lines 0 comments ? errors Download
M webkit/media/webvideoframe_impl.h View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -0 lines 0 comments ? errors Download
M webkit/media/webvideoframe_impl.cc View 1 2 3 4 1 chunk +0 lines, -9 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 34
slavi
The patch fails on the try bots currently because it depends on http://src.chromium.org/viewvc/chrome?view=rev&revision=163829 and the ...
1 year, 5 months ago #1
enne
You can do: git cl try --revision 163829
1 year, 5 months ago #2
danakj
https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw_quad.h File cc/video_frame_draw_quad.h (right): https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw_quad.h#newcode18 cc/video_frame_draw_quad.h:18: class VideoFrameDrawQuad : public DrawQuad { Any chance of ...
1 year, 5 months ago #3
slavi
On 2012/10/24 18:01:07, danakj wrote: > https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw_quad.h > File cc/video_frame_draw_quad.h (right): > > https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw_quad.h#newcode18 > ...
1 year, 5 months ago #4
danakj
On 2012/10/24 18:06:47, slavi wrote: > On 2012/10/24 18:01:07, danakj wrote: > > > https://chromiumcodereview.appspot.com/11274017/diff/2017/cc/video_frame_draw_quad.h ...
1 year, 5 months ago #5
slavi
On 2012/10/24 18:08:08, danakj wrote: > On 2012/10/24 18:06:47, slavi wrote: > > On 2012/10/24 ...
1 year, 5 months ago #6
danakj
On 2012/10/24 18:14:50, slavi wrote: > On 2012/10/24 18:08:08, danakj wrote: > > On 2012/10/24 ...
1 year, 5 months ago #7
slavi
> I think in order to avoid that the media code will have to draw ...
1 year, 5 months ago #8
danakj
On 2012/10/24 19:56:41, slavi wrote: > What do you think of adding VideoFrame resource type ...
1 year, 5 months ago #9
piman
+aelias. At a high level, being able to create a resource from existing data makes ...
1 year, 5 months ago #10
slavi
On 2012/10/24 22:48:41, piman wrote: > +aelias. > At a high level, being able to ...
1 year, 5 months ago #11
piman
On Wed, Oct 24, 2012 at 4:04 PM, <skaslev@chromium.org> wrote: > On 2012/10/24 22:48:41, piman ...
1 year, 5 months ago #12
aelias
I agree we should try to keep resource types to a minimum. The resource provider ...
1 year, 5 months ago #13
Ami Fischman
FYI On Wed, Oct 24, 2012 at 4:05 PM, Antoine Labour <piman@chromium.org> wrote: > Why ...
1 year, 5 months ago #14
danakj
On Wed, Oct 24, 2012 at 7:25 PM, Ami Fischman <fischman@chromium.org> wrote: > FYI > ...
1 year, 5 months ago #15
slavi
> Why can't we have the SkBitmaps take ownership of the VideoFrame buffers, > instead ...
1 year, 5 months ago #16
danakj
On Wed, Oct 24, 2012 at 7:29 PM, Dana Jansens <danakj@chromium.org> wrote: > On Wed, ...
1 year, 5 months ago #17
slavi
PTAL I moved the YUV -> RGB conversion to VideoLayerImpl and reuse TextureDrawQuad to transfer ...
1 year, 5 months ago #18
slavi
Added Ami Fischman for review of webkit/media changes.
1 year, 5 months ago #19
Ami Fischman
Only looked at webkit/media http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_impl.h File webkit/media/webvideoframe_impl.h (right): http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_impl.h#newcode18 webkit/media/webvideoframe_impl.h:18: WebVideoFrame* web_video_frame); optional: Personally I ...
1 year, 5 months ago #20
slavi
http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_impl.h File webkit/media/webvideoframe_impl.h (right): http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_impl.h#newcode18 webkit/media/webvideoframe_impl.h:18: WebVideoFrame* web_video_frame); On 2012/10/26 22:45:09, Ami Fischman wrote: > ...
1 year, 5 months ago #21
jamesr (out of office)
On 2012/10/26 23:46:23, slavi wrote: > http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_impl.h > File webkit/media/webvideoframe_impl.h (right): > > http://codereview.chromium.org/11274017/diff/5005/webkit/media/webvideoframe_impl.h#newcode18 > ...
1 year, 5 months ago #22
slavi
> Why can't you downcast from somewhere inside of webkit/compositor_bindings/, > perhaps by wrapping the ...
1 year, 5 months ago #23
slavi
On 2012/10/26 23:57:13, slavi wrote: > > Why can't you downcast from somewhere inside of ...
1 year, 5 months ago #24
Ami Fischman
I don't think it's ok to avoid componentization by copy/pasting (which is what you're having ...
1 year, 5 months ago #25
skaslev
The reason base::Bind doesn't save me is because everything in webkit/compositor_bindings (where base::Bind is called) ...
1 year, 5 months ago #26
slavi
> > I don't think it's ok to avoid componentization by copy/pasting (which is > ...
1 year, 5 months ago #27
Ami Fischman
If you can kill WebVideoFrame in the near future, and add a TODO explaining that ...
1 year, 5 months ago #28
slavi
On 2012/10/28 04:23:08, Ami Fischman wrote: > If you can kill WebVideoFrame in the near ...
1 year, 5 months ago #29
Ami Fischman
http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_impl.h File webkit/media/webvideoframe_impl.h (right): http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_impl.h#newcode37 webkit/media/webvideoframe_impl.h:37: // WebKit::WebVideoFrame and WebVideoFrameImpl which are currently unused. I'm ...
1 year, 5 months ago #30
Ami Fischman
http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_impl.h File webkit/media/webvideoframe_impl.h (right): http://codereview.chromium.org/11274017/diff/4005/webkit/media/webvideoframe_impl.h#newcode37 webkit/media/webvideoframe_impl.h:37: // WebKit::WebVideoFrame and WebVideoFrameImpl which are currently unused. On ...
1 year, 5 months ago #31
jamesr (out of office)
The goal here for cc is to not depend on any WebKit headers, types, libraries, ...
1 year, 5 months ago #32
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skaslev@chromium.org/11274017/41004
1 year, 5 months ago #33
I haz the power (commit-bot)
1 year, 5 months ago #34
Change committed as 164871
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6