I think emircan@ chose to use ARGBToI420() in https://codereview.chromium.org/1394783004

For [2] in #1, I found it also used SkBitmap.

dcastagna@: can you explain some difference between SkBitmap and SkPixmap?

#1: sorry, my bad, when I meant to use an example
of SkSurface + SkPixmap (the new thing) versus using
SkBitmap (deprecated), I meant:
https://cs.chromium.org/chromium/src/content/renderer/media/image_capture_frame_grabber.cc?q=framegrabber&sq=package:chromium&dr=CSs&l=72

I just worked on replacing SkBitmap usage with SkImage in video to canvas code.

I think SkBitmap usage is discouraged. I'd guess SkPixmap is the new way of accessing pixel storage?
Miguel's comment seems to agree with my guess.

Just noticed this Copy() is a two step copy.
Namely from |frame| --> |bitmap|, |bitmap| --> |new_frame|

The first step is at 
https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms_compositor.cc?dr=C&q=webmediaplayer_ms_compositor&sq=package:chromium&l=59

Question: Has SkCanvasVideoRenderer::Copy() already resolved the color format for the input frame?

dcastagna@ and emircan@ can you take a look? I am not very familiar with the Sk stuff.

SkCanvasVideoRenderer doesn't know anything about videoframe until Copy is called. It has a cache in case copy is called multiple times with the same videoframe.

I'm not sure what format you're referring to. The code you linked to is inside an if(video_frame->HasTextures()) branch. At that point there is only a pixel format for the textures in the videoframe, but that shouldn't affect the format you use to read back from the canvas.

I think mcasas is talking about the SkCanvas format, right?

qiangchen@ can you please clarify your statement in #4
"Question: Has SkCanvasVideoRenderer::Copy() already resolved the color format for the input frame?"
?

Please note that:
- This bug is _not_ about SkCanvasVideoRenderer at all.

- (The second part of) this bug description is about using SkSurface and SkPixmap instead of SkBitmap.

- But notice that in both cases, there is SkCanvas involved.

- SkCanvas is _not_ deprecated.

My Question is

For this statement:
 "video_renderer->Copy(frame.get(), &canvas, context_3d);"

Does this Copy call take |frame->format()| into consideration and do a conversion when painting onto |canvas|?

P.S. |video_renderer| is an instance of SkCanvasVideoRenderer.

I think I misunderstand this bug. I focused on the color conversion between input and output frame.

But this bug is saying:
1. The color format of SkBitmap (or SkPixmap) actually depends on kN32_SkColorType;
2. Thus the copy from SkBitmap to |new_frame| should not use libyuv::ARGBToI420, but should use ConvertToI420 with correct SkBitmap color format as source format.

Is this understanding correct?

#8: your understanding is part 1, an is correct.

There is a second part in the bug description, below the line:
--------------------------------------------------------
And that says that the code should be refactored to use
SkPixmap and SkSurface instead of SkBitmap.

qiangchen@ never mind, I'll take this one.

https://codereview.chromium.org/2153093003/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e2816a491e5f15d5fdfc809e7bad74f0cfcb9cb

commit 8e2816a491e5f15d5fdfc809e7bad74f0cfcb9cb
Author: mcasas <mcasas@chromium.org>
Date: Mon Jul 18 20:44:17 2016

WebMediaPlayerMsCompositor: use correct ARGB/ABGR, use SkSurface/SkPixmap ISO SkBitmap

Tot has ARGB hardcoded, whereas certain platforms (e.g.
Android) usually have ABGR. Make that decision runtime.

Also, code in ToT uses SkBitmap, which is discouraged,
this CL changes it to using SkSurface and SkPixmap
instead.

BUG= 628466 

Review-Url: https://codereview.chromium.org/2153093003
Cr-Commit-Position: refs/heads/master@{#406086}

[modify] https://crrev.com/8e2816a491e5f15d5fdfc809e7bad74f0cfcb9cb/content/renderer/media/webmediaplayer_ms_compositor.cc