Moved renderer implementation from media/filters/ to media/renderers/

Moved renderer implementation from media/filters/ to media/renderers/

Per dalecurtis@ suggestion on
https://codereview.chromium.org/883353012/

BUG=457959
Committed: https://crrev.com/f54f5c8f20c5e7ea09514a1deb2f67650b1a6240
Cr-Commit-Position: refs/heads/master@{#317876}

[servolk, 2015-02-19 23:24:15]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, gunsch@chromium.org, lcwu@chromium.org,
scherkus@chromium.org, wolenetz@chromium.org

[DaleCurtis, 2015-02-19 23:32:12]

I know scherkus@ has opinions in this area so I'll let him chime in, but
otherwise looks good to me.

[gunsch, 2015-02-19 23:44:15]

https://codereview.chromium.org/941633004/diff/1/media/renderers/renderer_impl.h
File media/renderers/renderer_impl.h (right):

https://codereview.chromium.org/941633004/diff/1/media/renderers/renderer_imp...
media/renderers/renderer_impl.h:5: #ifndef MEDIA_FILTERS_RENDERER_IMPL_H_
here and elsewhere: MEDIA_FILTERS --> MEDIA_RENDERERS

[servolk, 2015-02-19 23:48:16]

On 2015/02/19 23:44:15, gunsch wrote:
>
https://codereview.chromium.org/941633004/diff/1/media/renderers/renderer_impl.h
> File media/renderers/renderer_impl.h (right):
> 
>
https://codereview.chromium.org/941633004/diff/1/media/renderers/renderer_imp...
> media/renderers/renderer_impl.h:5: #ifndef MEDIA_FILTERS_RENDERER_IMPL_H_
> here and elsewhere: MEDIA_FILTERS --> MEDIA_RENDERERS

Done. Should we also update the copyright to 2015 while moving those files?

[servolk, 2015-02-19 23:49:52]

servolk@chromium.org changed reviewers:
+ aelias@chromium.org, bbudge@chromium.org, piman@chromium.org

[scherkus (not reviewing), 2015-02-20 17:54:12]

overall this is a good thing and thank you for tackling this!

the one class that shouldn't be moved is SkCanvasVideoRenderer ... despite the
name it's more of a utility class used by content/ code for painting a
VideoFrame to a SkCanvas and isn't used by anything under media/renderers/

[piman, 2015-02-20 18:15:46]

On 2015/02/20 17:54:12, scherkus wrote:
> overall this is a good thing and thank you for tackling this!
> 
> the one class that shouldn't be moved is SkCanvasVideoRenderer ... despite the
> name it's more of a utility class used by content/ code for painting a
> VideoFrame to a SkCanvas and isn't used by anything under media/renderers/

If it's only used by content/ it's fine to have it inside of content/ close to
where it's used.

[scherkus (not reviewing), 2015-02-20 18:16:58]

On 2015/02/20 18:15:46, piman (Very slow to review) wrote:
> On 2015/02/20 17:54:12, scherkus wrote:
> > overall this is a good thing and thank you for tackling this!
> > 
> > the one class that shouldn't be moved is SkCanvasVideoRenderer ... despite
the
> > name it's more of a utility class used by content/ code for painting a
> > VideoFrame to a SkCanvas and isn't used by anything under media/renderers/
> 
> If it's only used by content/ it's fine to have it inside of content/ close to
> where it's used.

(and by content/, I now mean media/blink/ code since we moved it all... so yeah
I think it's fine where it is for now)

[servolk, 2015-02-23 18:05:08]

On 2015/02/20 18:16:58, scherkus wrote:
> On 2015/02/20 18:15:46, piman (Very slow to review) wrote:
> > On 2015/02/20 17:54:12, scherkus wrote:
> > > overall this is a good thing and thank you for tackling this!
> > > 
> > > the one class that shouldn't be moved is SkCanvasVideoRenderer ... despite
> the
> > > name it's more of a utility class used by content/ code for painting a
> > > VideoFrame to a SkCanvas and isn't used by anything under media/renderers/
> > 
> > If it's only used by content/ it's fine to have it inside of content/ close
to
> > where it's used.
> 
> (and by content/, I now mean media/blink/ code since we moved it all... so
yeah
> I think it's fine where it is for now)

Ok. I see how SkCanvasVideoRenderer is indeed a renderer in some sense, but it
doesn't implement media::Renderer interface, so it's not a fully-featured
renderer. So what should we do about it - keep it in media/filters/ or move it
to media/blink/ ?

[scherkus (not reviewing), 2015-02-23 18:08:40]

Either works for me. media/blink/ is slightly better I suppose.
On Feb 23, 2015 10:05 AM, <servolk@chromium.org> wrote:

> On 2015/02/20 18:16:58, scherkus wrote:
>
>> On 2015/02/20 18:15:46, piman (Very slow to review) wrote:
>> > On 2015/02/20 17:54:12, scherkus wrote:
>> > > overall this is a good thing and thank you for tackling this!
>> > >
>> > > the one class that shouldn't be moved is SkCanvasVideoRenderer ...
>> despite
>> the
>> > > name it's more of a utility class used by content/ code for painting a
>> > > VideoFrame to a SkCanvas and isn't used by anything under
>> media/renderers/
>> >
>> > If it's only used by content/ it's fine to have it inside of content/
>> close
>>
> to
>
>> > where it's used.
>>
>
>  (and by content/, I now mean media/blink/ code since we moved it all... so
>>
> yeah
>
>> I think it's fine where it is for now)
>>
>
> Ok. I see how SkCanvasVideoRenderer is indeed a renderer in some sense,
> but it
> doesn't implement media::Renderer interface, so it's not a fully-featured
> renderer. So what should we do about it - keep it in media/filters/ or
> move it
> to media/blink/ ?
>
> https://codereview.chromium.org/941633004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[servolk, 2015-02-23 19:35:05]

On 2015/02/23 18:08:40, scherkus wrote:
> Either works for me. media/blink/ is slightly better I suppose.
> On Feb 23, 2015 10:05 AM, <mailto:servolk@chromium.org> wrote:
> 
> > On 2015/02/20 18:16:58, scherkus wrote:
> >
> >> On 2015/02/20 18:15:46, piman (Very slow to review) wrote:
> >> > On 2015/02/20 17:54:12, scherkus wrote:
> >> > > overall this is a good thing and thank you for tackling this!
> >> > >
> >> > > the one class that shouldn't be moved is SkCanvasVideoRenderer ...
> >> despite
> >> the
> >> > > name it's more of a utility class used by content/ code for painting a
> >> > > VideoFrame to a SkCanvas and isn't used by anything under
> >> media/renderers/
> >> >
> >> > If it's only used by content/ it's fine to have it inside of content/
> >> close
> >>
> > to
> >
> >> > where it's used.
> >>
> >
> >  (and by content/, I now mean media/blink/ code since we moved it all... so
> >>
> > yeah
> >
> >> I think it's fine where it is for now)
> >>
> >
> > Ok. I see how SkCanvasVideoRenderer is indeed a renderer in some sense,
> > but it
> > doesn't implement media::Renderer interface, so it's not a fully-featured
> > renderer. So what should we do about it - keep it in media/filters/ or
> > move it
> > to media/blink/ ?
> >
> > https://codereview.chromium.org/941633004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Done

[DaleCurtis, 2015-02-23 19:46:47]

everything else lgtm

https://codereview.chromium.org/941633004/diff/40001/media/renderers/audio_re...
File media/renderers/audio_renderer_algorithm.h (right):

https://codereview.chromium.org/941633004/diff/40001/media/renderers/audio_re...
media/renderers/audio_renderer_algorithm.h:21: #ifndef
MEDIA_RENDERERS_AUDIO_RENDERER_ALGORITHM_H_
Hmm, this is an implementation detail of the AudioRenderer and should probably
go somewhere else.  I'm fine with leaving it in media/filters for now, although
it's really a means for implementing playback rate changes so it could be
renamed and put in media/base.

[servolk, 2015-02-23 19:57:47]

On 2015/02/23 19:46:47, DaleCurtis wrote:
> everything else lgtm
> 
>
https://codereview.chromium.org/941633004/diff/40001/media/renderers/audio_re...
> File media/renderers/audio_renderer_algorithm.h (right):
> 
>
https://codereview.chromium.org/941633004/diff/40001/media/renderers/audio_re...
> media/renderers/audio_renderer_algorithm.h:21: #ifndef
> MEDIA_RENDERERS_AUDIO_RENDERER_ALGORITHM_H_
> Hmm, this is an implementation detail of the AudioRenderer and should probably
> go somewhere else.  I'm fine with leaving it in media/filters for now,
although
> it's really a means for implementing playback rate changes so it could be
> renamed and put in media/base.

Ok, moved audio_renderer_algorithm* back to filters/ for now.

[bbudge, 2015-02-23 21:30:39]

pepper LGTM

[scherkus (not reviewing), 2015-02-24 00:00:11]

lgtm
On Feb 23, 2015 1:30 PM, <bbudge@chromium.org> wrote:

> pepper LGTM
>
> https://codereview.chromium.org/941633004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wolenetz, 2015-02-24 00:04:14]

lgtm % tiny nit:

https://codereview.chromium.org/941633004/diff/60001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/941633004/diff/60001/media/media.gyp#newcode1214
media/media.gyp:1214: 'filters/audio_renderer_algorithm_unittest.cc',
nit: this is misplaced in alphabetical order now.

[servolk, 2015-02-24 00:13:11]

On 2015/02/24 00:04:14, wolenetz wrote:
> lgtm % tiny nit:
> 
> https://codereview.chromium.org/941633004/diff/60001/media/media.gyp
> File media/media.gyp (right):
> 
>
https://codereview.chromium.org/941633004/diff/60001/media/media.gyp#newcode1214
> media/media.gyp:1214: 'filters/audio_renderer_algorithm_unittest.cc',
> nit: this is misplaced in alphabetical order now.

thanks, fixed.

[servolk, 2015-02-24 00:34:02]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-02-24 00:34:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, bbudge@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/941633004/#ps80001 (title:
"Fixed alphabetical file order in media.gyp")

[commit-bot: I haz the power, 2015-02-24 00:35:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/941633004/80001

[commit-bot: I haz the power, 2015-02-24 00:42:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-24 00:42:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[servolk, 2015-02-24 02:08:26]

servolk@chromium.org changed reviewers:
+ aa@chromium.org

[servolk, 2015-02-24 03:37:23]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-02-24 03:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
dalecurtis@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/941633004/#ps180001
(title: "Fixed android build")

[commit-bot: I haz the power, 2015-02-24 03:37:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/941633004/180001

[servolk, 2015-02-24 03:44:47]

servolk@chromium.org changed reviewers:
+ avi@chromium.org, ccameron@chromium.org, creis@chromium.org,
darin@chromium.org, enne@chromium.org, jam@chromium.org, jamesr@chromium.org,
jochen@chromium.org, nasko@chromium.org, nduca@chromium.org

[servolk, 2015-02-24 03:46:28]

servolk@chromium.org changed reviewers:
+ ben@chromium.org

[servolk, 2015-02-24 03:46:43]

On 2015/02/24 03:37:51, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/941633004/180001

+a few more people for quick cc/, content/ and mojo/ sign off

[commit-bot: I haz the power, 2015-02-24 03:56:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-24 03:56:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jochen (gone - plz use gerrit), 2015-02-24 10:25:30]

content lgtm

please update the CL description (you're moving the files to media/blink not
media/renderer)

[servolk, 2015-02-24 17:49:41]

On 2015/02/24 10:25:30, jochen (slow) wrote:
> content lgtm
> 
> please update the CL description (you're moving the files to media/blink not
> media/renderer)

Thanks for the review! But I believe the CL description is correct. The primary
goal of this CL was to move renderer implementations into media/renderers/ and
that's what the first patchset did. Only later did scherkus@ suggest that the
better place for skcanvas_video_renderer is media/blink/, and so in later
patchsets I've taken that suggestion into account.

[jamesr, 2015-02-24 20:25:57]

cc/ and mojo/ lgtm

[servolk, 2015-02-24 20:27:02]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2015-02-24 20:27:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/941633004/180001

[commit-bot: I haz the power, 2015-02-24 20:32:49]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-24 20:33:35]

Patchset 10 (id:??) landed as
https://crrev.com/f54f5c8f20c5e7ea09514a1deb2f67650b1a6240
Cr-Commit-Position: refs/heads/master@{#317876}