Pass SkPaint instead of its alpha and mode in WebMediaPlayer::paint

media/renderers/skcanvas_video_renderer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/renderers/skcanvas_video_renderer.h"
 
 #include <limits>
 
 #include "base/macros.h"
 #include "gpu/GLES2/gl2extchromium.h"
@@ skipping 314 matching lines @@
           this,
           &SkCanvasVideoRenderer::ResetCache) {}
 
 SkCanvasVideoRenderer::~SkCanvasVideoRenderer() {
   ResetCache();
 }
 
 void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
                                   SkCanvas* canvas,
                                   const gfx::RectF& dest_rect,
-                                  uint8_t alpha,
-                                  SkXfermode::Mode mode,
+                                  const SkPaint* paint,

[Justin Novosad, 2016/08/25 19:18:18]

since the code is not mean to support nullptr for paint, you should use &
instead of *

                                   VideoRotation video_rotation,
                                   const Context3D& context_3d) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (alpha == 0) {
+  if (paint->getAlpha() == 0) {
     return;
   }
 
   SkRect dest;
   dest.set(dest_rect.x(), dest_rect.y(), dest_rect.right(), dest_rect.bottom());
 
-  SkPaint paint;
-  paint.setAlpha(alpha);
+  SkPaint freshPaint;

[Justin Novosad, 2016/08/25 19:18:19]

this is a funny name.  What do you mean by fresh?

[xidachen, 2016/08/26 02:21:39]

It is basically explaining that we are creating a new skpaint instead of using
the one that is passed in. Should I name it "newPaint"?

[Justin Novosad, 2016/08/26 14:25:25]

Think of the person reading this code trying to understand it.  There is a
reason you are not using "paint" directly.  You only want to use part of the
settings in "paint".  This code sets up a paint and changes it further later on,
which is also bad for readability.  I would encourage you to us two paint
objects.  Call the first one  blackWithAlphaPaint, and create it inside the "if"
statement that uses it. The second one could be videoPaint, or something like
that to imply that it was construct with specifically the properties needed for
the video blit.

+  freshPaint.setAlpha(paint->getAlpha());
 
   // Paint black rectangle if there isn't a frame available or the
   // frame has an unexpected format.
   if (!video_frame.get() || video_frame->natural_size().IsEmpty() ||
       !(media::IsYuvPlanar(video_frame->format()) ||
         video_frame->HasTextures())) {
-    canvas->drawRect(dest, paint);
+    canvas->drawRect(dest, freshPaint);
     canvas->flush();
     return;
   }
 
   gpu::gles2::GLES2Interface* gl = context_3d.gl;
   if (!UpdateLastImage(video_frame, context_3d))
     return;
 
-  paint.setXfermodeMode(mode);
-  paint.setFilterQuality(kLow_SkFilterQuality);
+  SkXfermode::Mode mode;
+  if (!paint || !SkXfermode::AsMode(paint->getXfermode(), &mode))
+    mode = SkXfermode::kSrcOver_Mode;
+  freshPaint.setXfermodeMode(mode);
+  freshPaint.setFilterQuality(paint->getFilterQuality());
 
   const bool need_rotation = video_rotation != VIDEO_ROTATION_0;
   const bool need_scaling =
       dest_rect.size() !=
       gfx::SizeF(gfx::SkISizeToSize(last_image_->dimensions()));
   const bool need_translation = !dest_rect.origin().IsOrigin();
   bool need_transform = need_rotation || need_scaling || need_translation;
   if (need_transform) {
     canvas->save();
     canvas->translate(
@@ skipping 29 matching lines @@
   }
 
   // This is a workaround for crbug.com/524717. A texture backed image is not
   // safe to access on another thread or GL context. So if we're drawing into a
   // recording canvas we read the texture back into CPU memory and record that
   // sw image into the SkPicture. The long term solution is for Skia to provide
   // a SkPicture filter that makes a picture safe for multiple CPU raster
   // threads. (skbug.com/4321).
   if (canvas->imageInfo().colorType() == kUnknown_SkColorType) {
     sk_sp<SkImage> swImage = last_image_->makeNonTextureImage();
-    canvas->drawImage(swImage, 0, 0, &paint);
+    canvas->drawImage(swImage, 0, 0, &freshPaint);
   } else {
-    canvas->drawImage(last_image_.get(), 0, 0, &paint);
+    canvas->drawImage(last_image_.get(), 0, 0, &freshPaint);
   }
 
   if (need_transform)
     canvas->restore();
   // Make sure to flush so we can remove the videoframe from the generator.
   canvas->flush();
 
   if (video_frame->HasTextures()) {
     DCHECK(gl);
     SyncTokenClientImpl client(gl);
     video_frame->UpdateReleaseSyncToken(&client);
   }
 }
 
 void SkCanvasVideoRenderer::Copy(const scoped_refptr<VideoFrame>& video_frame,
                                  SkCanvas* canvas,
                                  const Context3D& context_3d) {
-  Paint(video_frame, canvas, gfx::RectF(video_frame->visible_rect()), 0xff,
-        SkXfermode::kSrc_Mode, media::VIDEO_ROTATION_0, context_3d);
+  SkPaint paint;
+  paint.setXfermodeMode(SkXfermode::kSrc_Mode);
+  paint.setAlpha(0xFF);

[Justin Novosad, 2016/08/25 19:18:18]

not necessary

[xidachen, 2016/08/26 02:21:39]

Done.

+  paint.setFilterQuality(kLow_SkFilterQuality);

[Justin Novosad, 2016/08/25 19:18:18]

not necessary

[xidachen, 2016/08/26 02:21:39]

But without this line, the default filter quality is kNone.

+  Paint(video_frame, canvas, gfx::RectF(video_frame->visible_rect()), &paint,
+        media::VIDEO_ROTATION_0, context_3d);
 }
 
 namespace {
 
 // libyuv doesn't support 9- and 10-bit video frames yet. This function
 // creates a regular 8-bit video frame which we can give to libyuv.
 scoped_refptr<VideoFrame> DownShiftHighbitVideoFrame(
     const VideoFrame* video_frame) {
   VideoPixelFormat format;
   int shift = 1;
@@ skipping 301 matching lines @@
     if (!last_image_)  // Couldn't create the SkImage.
       return false;
     last_timestamp_ = video_frame->timestamp();
   }
   last_image_deleting_timer_.Reset();
   DCHECK(!!last_image_);
   return true;
 }
 
 }  // namespace media