Moved SkCanvasVideoRenderer to media/filters.

Moved SkCanvasVideoRenderer to media/filters.

SkCanvasVideoRenderer will be used for rendering YUV video streams by
the software compositor. To avoid circular dependencies cc/ cannot depend
on webkit/ hence the move.

Also changed the interface of SkCanvasVideoRenderer::Paint to accept
gfx::RectF instead of gfx::Rect. The gfx::Rect was converted to a
floating point SkRect anyway and we actually use the floating point
version from the software compositor.


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

[Ami GONE FROM CHROMIUM, 2012-10-23 23:44:09]

I think this is basically fine.

http://codereview.chromium.org/11234071/diff/1/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode29
media/media.gyp:29: '../skia/skia.gyp:skia',
here and below is depending on skia/ instead of third_party/skia/ really the
right thing?  
If you do do this, ISTM you need to edit media/DEPS as well to keep checkdeps
happy.

http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode437
media/media.gyp:437: '../skia/skia.gyp:skia',
droppable?

[slavi, 2012-10-23 23:53:29]

On 2012/10/23 23:44:09, Ami Fischman wrote:
> I think this is basically fine.
> 
> http://codereview.chromium.org/11234071/diff/1/media/media.gyp
> File media/media.gyp (right):
> 
> http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode29
> media/media.gyp:29: '../skia/skia.gyp:skia',
> here and below is depending on skia/ instead of third_party/skia/ really the
> right thing?  
> If you do do this, ISTM you need to edit media/DEPS as well to keep checkdeps
> happy.

Skia is already in media/DEPS.

Not sure about '../skia/skia.gyp:skia' vs 'third_party/skia'. But
'../skia/skia.gyp:skia' is how Skia is referenced from cc/ and webkit/ which I
merely imitated.

> http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode437
> media/media.gyp:437: '../skia/skia.gyp:skia',
> droppable?

I'm giving it a try. Hopefully the trybots will like it.

[Ami GONE FROM CHROMIUM, 2012-10-23 23:55:45]

> 
http://codereview.chromium.**org/11234071/diff/1/media/**media.gyp<http://cod...
>> File media/media.gyp (right):
>
>  http://codereview.chromium.**org/11234071/diff/1/media/**
>>
media.gyp#newcode29<http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode29>
>> media/media.gyp:29: '../skia/skia.gyp:skia',
>> here and below is depending on skia/ instead of third_party/skia/ really
>> the
>> right thing?
>> If you do do this, ISTM you need to edit media/DEPS as well to keep
>> checkdeps
>> happy.
>
> Skia is already in media/DEPS.

Not sure about '../skia/skia.gyp:skia' vs 'third_party/skia'. But
> '../skia/skia.gyp:skia' is how Skia is referenced from cc/ and webkit/
> which I
> merely imitated.


DEPS are per-directory, and skia/ is different from third_party/skia.
 media/DEPS only contains third_party/skia, not skia/.
Have you tried running
./tools/checkdeps/checkdeps
?

[slavi, 2012-10-24 00:01:21]

On 2012/10/23 23:55:45, Ami Fischman wrote:
> > 
>
http://codereview.chromium.**org/11234071/diff/1/media/**media.gyp%3Chttp://c...>
> >> File media/media.gyp (right):
> >
> >  http://codereview.chromium.**org/11234071/diff/1/media/**
> >>
>
media.gyp#newcode29<http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode29>
> >> media/media.gyp:29: '../skia/skia.gyp:skia',
> >> here and below is depending on skia/ instead of third_party/skia/ really
> >> the
> >> right thing?
> >> If you do do this, ISTM you need to edit media/DEPS as well to keep
> >> checkdeps
> >> happy.
> >
> > Skia is already in media/DEPS.
> 
> Not sure about '../skia/skia.gyp:skia' vs 'third_party/skia'. But
> > '../skia/skia.gyp:skia' is how Skia is referenced from cc/ and webkit/
> > which I
> > merely imitated.
> 
> 
> DEPS are per-directory, and skia/ is different from third_party/skia.
>  media/DEPS only contains third_party/skia, not skia/.
> Have you tried running
> ./tools/checkdeps/checkdeps
> ?

Checkdeps succeeds on my checkout besides some Android java warning:

skaslev@skaslev2:~/src/chrome/src$ ./tools/checkdeps/checkdeps.py 
Using base directory: /usr/local/google/home/skaslev/src/chrome/src
Checking: /usr/local/google/home/skaslev/src/chrome/src
WARNING: multiple definitions of com.example.HelloLicenseServlet:
   
/usr/local/google/home/skaslev/src/chrome/src/chrome/common/extensions/docs/examples/apps/hello-java/workspace/HelloLicense/src/com/example/HelloLicenseServlet.java
   
/usr/local/google/home/skaslev/src/chrome/src/chrome/common/extensions/docs/examples/apps/hello-java/HelloLicenseServlet.java


SUCCESS

[slavi, 2012-10-24 00:09:10]

On 2012/10/24 00:01:21, slavi wrote:
> On 2012/10/23 23:55:45, Ami Fischman wrote:
> > > 
> >
>
http://codereview.chromium.**org/11234071/diff/1/media/**media.gyp%253Chttp:/...>
> > >> File media/media.gyp (right):
> > >
> > >  http://codereview.chromium.**org/11234071/diff/1/media/**
> > >>
> >
>
media.gyp#newcode29<http://codereview.chromium.org/11234071/diff/1/media/media.gyp#newcode29>
> > >> media/media.gyp:29: '../skia/skia.gyp:skia',
> > >> here and below is depending on skia/ instead of third_party/skia/ really
> > >> the
> > >> right thing?
> > >> If you do do this, ISTM you need to edit media/DEPS as well to keep
> > >> checkdeps
> > >> happy.
> > >
> > > Skia is already in media/DEPS.
> > 
> > Not sure about '../skia/skia.gyp:skia' vs 'third_party/skia'. But
> > > '../skia/skia.gyp:skia' is how Skia is referenced from cc/ and webkit/
> > > which I
> > > merely imitated.
> > 
> > 
> > DEPS are per-directory, and skia/ is different from third_party/skia.
> >  media/DEPS only contains third_party/skia, not skia/.
> > Have you tried running
> > ./tools/checkdeps/checkdeps
> > ?
> 
> Checkdeps succeeds on my checkout besides some Android java warning:
> 
> skaslev@skaslev2:~/src/chrome/src$ ./tools/checkdeps/checkdeps.py 
> Using base directory: /usr/local/google/home/skaslev/src/chrome/src
> Checking: /usr/local/google/home/skaslev/src/chrome/src
> WARNING: multiple definitions of com.example.HelloLicenseServlet:
>    
>
/usr/local/google/home/skaslev/src/chrome/src/chrome/common/extensions/docs/examples/apps/hello-java/workspace/HelloLicense/src/com/example/HelloLicenseServlet.java
>    
>
/usr/local/google/home/skaslev/src/chrome/src/chrome/common/extensions/docs/examples/apps/hello-java/HelloLicenseServlet.java
> 
> 
> SUCCESS

Adding piman as reviewer because of the needed changes to content/.

[Ami GONE FROM CHROMIUM, 2012-10-24 00:09:45]

On Tue, Oct 23, 2012 at 5:01 PM, <skaslev@chromium.org> wrote:

> Checkdeps succeeds on my checkout besides some Android java warning:
>

OIC - media/ picks up skia/ transitively through ui/.

LGTM for media/

[piman, 2012-10-24 00:14:43]

lgtm

[DaleCurtis, 2012-10-24 00:20:00]

lgtm

[commit-bot: I haz the power, 2012-10-24 00:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11234071/7003

[DaleCurtis, 2012-10-24 00:24:39]

On 2012/10/24 00:20:00, DaleCurtis wrote:
> lgtm

win_rel shows a bunch of ffmpeg failures while trying to compile inttypes.h.
Does your skia.gyp include anything that's overriding these types?

[DaleCurtis, 2012-10-24 00:26:21]

On 2012/10/24 00:24:39, DaleCurtis wrote:
> On 2012/10/24 00:20:00, DaleCurtis wrote:
> > lgtm
> 
> win_rel shows a bunch of ffmpeg failures while trying to compile inttypes.h.
> Does your skia.gyp include anything that's overriding these types?

To answer my own question, yes:
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/include/c...

[slavi, 2012-10-24 00:28:59]

On 2012/10/24 00:26:21, DaleCurtis wrote:
> On 2012/10/24 00:24:39, DaleCurtis wrote:
> > On 2012/10/24 00:20:00, DaleCurtis wrote:
> > > lgtm
> > 
> > win_rel shows a bunch of ffmpeg failures while trying to compile inttypes.h.
> > Does your skia.gyp include anything that's overriding these types?
> 
> To answer my own question, yes:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/include/c...

I was looking at the same thing. The error was from
media\filters\ffmpeg_h264_to_annex_b_bitstream_converter.cc which doesn't
include skia headers at all.

Could it be a bot flake?

[DaleCurtis, 2012-10-24 00:33:48]

On 2012/10/24 00:28:59, slavi wrote:
> On 2012/10/24 00:26:21, DaleCurtis wrote:
> > On 2012/10/24 00:24:39, DaleCurtis wrote:
> > > On 2012/10/24 00:20:00, DaleCurtis wrote:
> > > > lgtm
> > > 
> > > win_rel shows a bunch of ffmpeg failures while trying to compile
inttypes.h.
> > > Does your skia.gyp include anything that's overriding these types?
> > 
> > To answer my own question, yes:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/include/c...
> 
> I was looking at the same thing. The error was from
> media\filters\ffmpeg_h264_to_annex_b_bitstream_converter.cc which doesn't
> include skia headers at all.
> 
> Could it be a bot flake?

No it's not flake, the skia code is overriding some of the ffmpeg headers via
that file I linked. You'll need to exclude that when compiling the media code.
I'm not sure how it's getting injected right now.

[skaslev, 2012-10-24 00:41:55]

Then again there's
src/skia/config/SkUserConfig.h<http://code.google.com/searchframe#OAMlx_jo-ck/src/skia/config/SkUserConfig.h&exact_package=chromium&l=196>
which
also defines those types.

I'll investigate.


On Tue, Oct 23, 2012 at 5:33 PM, <dalecurtis@chromium.org> wrote:

> On 2012/10/24 00:28:59, slavi wrote:
>
>> On 2012/10/24 00:26:21, DaleCurtis wrote:
>> > On 2012/10/24 00:24:39, DaleCurtis wrote:
>> > > On 2012/10/24 00:20:00, DaleCurtis wrote:
>> > > > lgtm
>> > >
>> > > win_rel shows a bunch of ffmpeg failures while trying to compile
>>
> inttypes.h.
>
>> > > Does your skia.gyp include anything that's overriding these types?
>> >
>> > To answer my own question, yes:
>> >
>>
>
> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> third_party/skia/include/**config/sk_stdint.h&exact_**
>
package=chromium&ct=rc&cd=1&q=**sk_stdint.h&sq=<http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/include/config/sk_stdint.h&exact_package=chromium&ct=rc&cd=1&q=sk_stdint.h&sq=>
>
>  I was looking at the same thing. The error was from
>> media\filters\ffmpeg_h264_to_**annex_b_bitstream_converter.cc which
>> doesn't
>> include skia headers at all.
>>
>
>  Could it be a bot flake?
>>
>
> No it's not flake, the skia code is overriding some of the ffmpeg headers
> via
> that file I linked. You'll need to exclude that when compiling the media
> code.
> I'm not sure how it's getting injected right now.
>
>
http://codereview.chromium.**org/11234071/<http://codereview.chromium.org/112...
>

[slavi, 2012-10-24 03:01:46]

Adding Mike Reed for review of the removal of skia/config/win/stdint.h.

The reason for the removal was it gets picked by
third_party/ffmpeg/chromium/include/win/inttypes.h which then tries to use
intmax_t which is not defined.

Also, it turns out that skia/config/win/stdint.h is not used by Skia in Chrome
at all since src/skia/config/SkUserConfig.h defines SK_IGNORE_STDINT_DOT_H and
supplies the types that Skia needs
http://code.google.com/searchframe#OAMlx_jo-ck/src/skia/config/SkUserConfig.h....

[slavi, 2012-10-24 03:03:27]

On 2012/10/24 03:01:46, slavi wrote:
> Adding Mike Reed for review of the removal of skia/config/win/stdint.h.
> 
> The reason for the removal was it gets picked by
> third_party/ffmpeg/chromium/include/win/inttypes.h which then tries to use
> intmax_t which is not defined.
> 
> Also, it turns out that skia/config/win/stdint.h is not used by Skia in Chrome
> at all since src/skia/config/SkUserConfig.h defines SK_IGNORE_STDINT_DOT_H and
> supplies the types that Skia needs
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/skia/config/SkUserConfig.h....

In Windows builds, that is.

[reed1, 2012-10-24 12:17:00]

removal looks good to me

[commit-bot: I haz the power, 2012-10-24 12:49:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11234071/34003

[commit-bot: I haz the power, 2012-10-24 15:13:32]

Change committed as 163829