Use SkSrc_Mode in SkCanvasVideoRenderer, to prevent transparent video frames from blending with reu…

Use SkSrc_Mode in SkCanvasVideoRenderer, to prevent transparent video frames from blending with reused image buffer contents.

BUG=392733
TEST=SkCanvasVideoRendererTest.TransparentFrame

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283338

[rileya (GONE FROM CHROMIUM), 2014-07-14 20:59:12]

Both the software compositor and the video->canvas paths seem to reuse image
buffers without clearing them, so video frames with alpha channels will
potentially get blended into their contents. This results in ugly visual
artifacts where a transparent video frame is blended with prior frames.

This fixes it by writing the new frame over the dest canvas, rather than
blending into it in SkCanvasVideoRenderer. This also may speed things up a bit
by eliminating the unnecessary blending math.

This has potential security implications noted in the bug:
https://code.google.com/p/chromium/issues/detail?id=392733 so I've set this to
private for the moment, while I look more closely at how buffers are reused.

[reed1, 2014-07-14 21:03:28]

lgtm from the skia perspective
... maybe a comment motivating it?

// use SRC mode so we completely overwrite the buffer (in case we have alpha)
// this means we don't need the extra cost of clearing the buffer first.

[scherkus (not reviewing), 2014-07-14 22:01:36]

think we can get a test?

[rileya (GONE FROM CHROMIUM), 2014-07-14 23:14:34]

Added a unit test and comment.

[scherkus (not reviewing), 2014-07-14 23:21:17]

https://codereview.chromium.org/388363002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/388363002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:223: EXPECT_EQ((const unsigned
int)SK_ColorTRANSPARENT, GetColor(target_canvas()));
we don't seem to cast elsewhere for SK_ColorRED ... do we really need it?

[rileya (GONE FROM CHROMIUM), 2014-07-14 23:35:10]

https://codereview.chromium.org/388363002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/388363002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:223: EXPECT_EQ((const unsigned
int)SK_ColorTRANSPARENT, GetColor(target_canvas()));
On 2014/07/14 23:21:17, scherkus wrote:
> we don't seem to cast elsewhere for SK_ColorRED ... do we really need it?

Without it, Clang complains about a signed vs unsigned compare. A cast to
SkColor would probably be better actually...

I think this is because SK_ColorTRANSPARENT is a #define of the literal
0x00000000 which the compiler decides to make signed, where the other SK_Color*
constants are 0xFFxxxxxx which the compiler seems to decide to be unsigned (due
to the leading 1 I guess). Combine that with picky gtest macros/templates, and
apparently we get compiler errors.

[scherkus (not reviewing), 2014-07-14 23:41:42]

https://codereview.chromium.org/388363002/diff/20001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/388363002/diff/20001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:223: EXPECT_EQ((const unsigned
int)SK_ColorTRANSPARENT, GetColor(target_canvas()));
On 2014/07/14 23:35:10, rileya wrote:
> On 2014/07/14 23:21:17, scherkus wrote:
> > we don't seem to cast elsewhere for SK_ColorRED ... do we really need it?
> 
> Without it, Clang complains about a signed vs unsigned compare. A cast to
> SkColor would probably be better actually...
> 
> I think this is because SK_ColorTRANSPARENT is a #define of the literal
> 0x00000000 which the compiler decides to make signed, where the other
SK_Color*
> constants are 0xFFxxxxxx which the compiler seems to decide to be unsigned
(due
> to the leading 1 I guess). Combine that with picky gtest macros/templates, and
> apparently we get compiler errors.

hmm... perhaps SkColor.h should fixed to define SK_ColorTRANSPARENT as
0x00000000U :)

in any case, this should be a static_cast<> as per style

[scherkus (not reviewing), 2014-07-14 23:41:51]

oh and lgtm

[rileya (GONE FROM CHROMIUM), 2014-07-15 18:31:58]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-07-15 18:35:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/388363002/40001

[commit-bot: I haz the power, 2014-07-16 04:45:01]

Change committed as 283338