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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 6 months ago by slavi
Modified:
2 years, 6 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, cc-bugs_chromium.org, darin-cc_chromium.org, piman (Very slow to review)
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) Patch
M cc/DEPS View 1 chunk +1 line, -0 lines 0 comments Download
M cc/cc.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M cc/cc_tests.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments 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 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 Download
M cc/software_renderer.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -1 line 0 comments 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 Download
M cc/video_layer.h View 1 2 2 chunks +14 lines, -3 lines 0 comments Download
M cc/video_layer.cc View 1 2 2 chunks +10 lines, -7 lines 0 comments Download
M cc/video_layer_impl.h View 1 2 3 4 5 6 7 5 chunks +21 lines, -8 lines 0 comments Download
M cc/video_layer_impl.cc View 1 2 3 4 5 6 12 chunks +107 lines, -61 lines 0 comments Download
M webkit/compositor_bindings/web_video_layer_impl.h View 1 chunk +0 lines, -1 line 0 comments Download
M webkit/compositor_bindings/web_video_layer_impl.cc View 1 2 2 chunks +8 lines, -3 lines 0 comments Download
M webkit/media/webvideoframe_impl.h View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -0 lines 0 comments Download
M webkit/media/webvideoframe_impl.cc View 1 2 3 4 1 chunk +0 lines, -9 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 34 (0 generated)
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 ...
2 years, 6 months ago (2012-10-24 17:51:09 UTC) #1
enne
You can do: git cl try --revision 163829
2 years, 6 months ago (2012-10-24 17:56:48 UTC) #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 ...
2 years, 6 months ago (2012-10-24 18:01:07 UTC) #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 > ...
2 years, 6 months ago (2012-10-24 18:06:47 UTC) #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 ...
2 years, 6 months ago (2012-10-24 18:08:08 UTC) #5
slavi
On 2012/10/24 18:08:08, danakj wrote: > On 2012/10/24 18:06:47, slavi wrote: > > On 2012/10/24 ...
2 years, 6 months ago (2012-10-24 18:14:50 UTC) #6
danakj
On 2012/10/24 18:14:50, slavi wrote: > On 2012/10/24 18:08:08, danakj wrote: > > On 2012/10/24 ...
2 years, 6 months ago (2012-10-24 18:20:27 UTC) #7
slavi
> I think in order to avoid that the media code will have to draw ...
2 years, 6 months ago (2012-10-24 19:56:41 UTC) #8
danakj
On 2012/10/24 19:56:41, slavi wrote: > What do you think of adding VideoFrame resource type ...
2 years, 6 months ago (2012-10-24 20:08:29 UTC) #9
piman (Very slow to review)
+aelias. At a high level, being able to create a resource from existing data makes ...
2 years, 6 months ago (2012-10-24 22:48:41 UTC) #10
slavi
On 2012/10/24 22:48:41, piman wrote: > +aelias. > At a high level, being able to ...
2 years, 6 months ago (2012-10-24 23:04:05 UTC) #11
piman (Very slow to review)
On Wed, Oct 24, 2012 at 4:04 PM, <skaslev@chromium.org> wrote: > On 2012/10/24 22:48:41, piman ...
2 years, 6 months ago (2012-10-24 23:06:03 UTC) #12
aelias_OOO_until_May29
I agree we should try to keep resource types to a minimum. The resource provider ...
2 years, 6 months ago (2012-10-24 23:10:45 UTC) #13
Ami GONE FROM CHROMIUM
FYI On Wed, Oct 24, 2012 at 4:05 PM, Antoine Labour <piman@chromium.org> wrote: > Why ...
2 years, 6 months ago (2012-10-24 23:25:54 UTC) #14
danakj
On Wed, Oct 24, 2012 at 7:25 PM, Ami Fischman <fischman@chromium.org> wrote: > FYI > ...
2 years, 6 months ago (2012-10-24 23:29:34 UTC) #15
slavi
> Why can't we have the SkBitmaps take ownership of the VideoFrame buffers, > instead ...
2 years, 6 months ago (2012-10-24 23:32:28 UTC) #16
danakj
On Wed, Oct 24, 2012 at 7:29 PM, Dana Jansens <danakj@chromium.org> wrote: > On Wed, ...
2 years, 6 months ago (2012-10-24 23:33:41 UTC) #17
slavi
PTAL I moved the YUV -> RGB conversion to VideoLayerImpl and reuse TextureDrawQuad to transfer ...
2 years, 6 months ago (2012-10-25 20:27:28 UTC) #18
slavi
Added Ami Fischman for review of webkit/media changes.
2 years, 6 months ago (2012-10-26 16:51:35 UTC) #19
Ami GONE FROM CHROMIUM
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 ...
2 years, 6 months ago (2012-10-26 22:45:09 UTC) #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: > ...
2 years, 6 months ago (2012-10-26 23:46:23 UTC) #21
jamesr
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 > ...
2 years, 6 months ago (2012-10-26 23:50:25 UTC) #22
slavi
> Why can't you downcast from somewhere inside of webkit/compositor_bindings/, > perhaps by wrapping the ...
2 years, 6 months ago (2012-10-26 23:57:13 UTC) #23
slavi
On 2012/10/26 23:57:13, slavi wrote: > > Why can't you downcast from somewhere inside of ...
2 years, 6 months ago (2012-10-26 23:58:44 UTC) #24
Ami GONE FROM CHROMIUM
I don't think it's ok to avoid componentization by copy/pasting (which is what you're having ...
2 years, 6 months ago (2012-10-27 03:46:01 UTC) #25
skaslev
The reason base::Bind doesn't save me is because everything in webkit/compositor_bindings (where base::Bind is called) ...
2 years, 6 months ago (2012-10-27 04:27:39 UTC) #26
slavi
> > I don't think it's ok to avoid componentization by copy/pasting (which is > ...
2 years, 6 months ago (2012-10-27 07:14:10 UTC) #27
Ami GONE FROM CHROMIUM
If you can kill WebVideoFrame in the near future, and add a TODO explaining that ...
2 years, 6 months ago (2012-10-28 04:23:08 UTC) #28
slavi
On 2012/10/28 04:23:08, Ami Fischman wrote: > If you can kill WebVideoFrame in the near ...
2 years, 6 months ago (2012-10-29 16:53:31 UTC) #29
Ami GONE FROM CHROMIUM
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 ...
2 years, 6 months ago (2012-10-29 16:57:25 UTC) #30
Ami GONE FROM CHROMIUM
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 ...
2 years, 6 months ago (2012-10-29 17:10:17 UTC) #31
jamesr
The goal here for cc is to not depend on any WebKit headers, types, libraries, ...
2 years, 6 months ago (2012-10-29 21:32:08 UTC) #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
2 years, 6 months ago (2012-10-30 04:17:38 UTC) #33
I haz the power (commit-bot)
2 years, 6 months ago (2012-10-30 06:45:51 UTC) #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 fa3abf7