Move rtc_video_decoder* from media/filter/ to content/renderer/media/.

The RTCVideoDecoder will eventially need to depend on third_party lib (webrtc, libjinle etc), which is not allowed in media. So move rtc_video_decoder* from media/filter/ to content/renderer/media/.

patch by ronghuawu@google.com

BUG=none
TEST=unit_tests, media_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91721

[Ronghua, 2011-06-16 20:47:39]

Hey Wei,

Can you help take a look first before I send out for review?

[wjia(left Chromium), 2011-06-16 21:49:38]

On 2011/06/16 20:47:39, ronghuawu wrote:
> Hey Wei,
> 
> Can you help take a look first before I send out for review?

it looks like IsUrlSupported would belong to some high level class, e.g.,
pipeline, or media_stream. It shouldn't be with RtcVideoDecoder.

pipeline_impl.cc shouldn't depend on rtc_video_decoder.h after the move. One way
is to change the interface of media::Pipeline::Start to have more information
about which video decoder the pipeline will pick up.

[Ronghua, 2011-06-16 23:28:15]

* Removed pipeline_impl.cc's dependency to rtc_video_decoder.h.
* Add "bool raw_video" to the Pipeline:Start interface.
* The IsUrlSupported is still remaining in the RTCVideoDecoder, but it should
eventually move to the media_stream when available.

[scherkus (not reviewing), 2011-06-17 00:31:46]

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:18: namespace media {
ditto

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.h:29: namespace media {
you're no longer inside media namespace

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder_unittest.cc:32: namespace media {
ditto

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:622: if (raw_video) {
this raw_video thing is kind of hacky....

can we perhaps inspect the scheme of the URL?

http://codereview.chromium.org/7193001/diff/3004/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7193001/diff/3004/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:12: #include
"content/renderer/media/rtc_video_decoder.h"
webkit glue can't depend on content

AFAIK you'll have to add this filter inside of render_view.cc

[Ronghua, 2011-06-23 17:42:26]

Hi Andrew,

We can create the filter inside of render_view.cc and pass to
webmediaplayer_**impl.cc via the FilterCollection. However in that case
there will be 2 VideoDecoder filters in the collection - the default
(ffmpeg) one and the rtc one. Later in pipeline_impl.cc,
the SelectVideoDecoder currently always returns the first VideoDecoder. Do
you think we may add one more parameter to SelectVideoDecoder? Something
like below. So that we can tell the SelectVideoDecoder select one particular
VideoDecoder in the FilterCollection based on the URL.

void SelectVideoDecoder(scoped_refptr<VideoDecoder>* filter_out,
VideoDecoderType
type);

Thanks,
Ronghua

On Thu, Jun 16, 2011 at 5:31 PM, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.**org/7193001/diff/3004/content/**
>
renderer/media/rtc_video_**decoder.cc<http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_video_decoder.cc>
> File content/renderer/media/rtc_**video_decoder.cc (right):
>
> http://codereview.chromium.**org/7193001/diff/3004/content/**
>
renderer/media/rtc_video_**decoder.cc#newcode18<http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_video_decoder.cc#newcode18>
> content/renderer/media/rtc_**video_decoder.cc:18: namespace media {
> ditto
>
> http://codereview.chromium.**org/7193001/diff/3004/content/**
>
renderer/media/rtc_video_**decoder.h<http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_video_decoder.h>
> File content/renderer/media/rtc_**video_decoder.h (right):
>
> http://codereview.chromium.**org/7193001/diff/3004/content/**
>
renderer/media/rtc_video_**decoder.h#newcode29<http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_video_decoder.h#newcode29>
> content/renderer/media/rtc_**video_decoder.h:29: namespace media {
> you're no longer inside media namespace
>
> http://codereview.chromium.**org/7193001/diff/3004/content/**
>
renderer/media/rtc_video_**decoder_unittest.cc<http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_video_decoder_unittest.cc>
> File content/renderer/media/rtc_**video_decoder_unittest.cc (right):
>
> http://codereview.chromium.**org/7193001/diff/3004/content/**
>
renderer/media/rtc_video_**decoder_unittest.cc#newcode32<http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_video_decoder_unittest.cc#newcode32>
> content/renderer/media/rtc_**video_decoder_unittest.cc:32: namespace media
> {
> ditto
>
> http://codereview.chromium.**org/7193001/diff/3004/media/**
>
base/pipeline_impl.cc<http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc>
> File media/base/pipeline_impl.cc (right):
>
> http://codereview.chromium.**org/7193001/diff/3004/media/**
>
base/pipeline_impl.cc#**newcode622<http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc#newcode622>
> media/base/pipeline_impl.cc:**622: if (raw_video) {
> this raw_video thing is kind of hacky....
>
> can we perhaps inspect the scheme of the URL?
>
> http://codereview.chromium.**org/7193001/diff/3004/webkit/**
>
glue/webmediaplayer_impl.cc<http://codereview.chromium.org/7193001/diff/3004/webkit/glue/webmediaplayer_impl.cc>
> File webkit/glue/webmediaplayer_**impl.cc (right):
>
> http://codereview.chromium.**org/7193001/diff/3004/webkit/**
>
glue/webmediaplayer_impl.cc#**newcode12<http://codereview.chromium.org/7193001/diff/3004/webkit/glue/webmediaplayer_impl.cc#newcode12>
> webkit/glue/webmediaplayer_**impl.cc:12: #include
>
> "content/renderer/media/rtc_**video_decoder.h"
> webkit glue can't depend on content
>
> AFAIK you'll have to add this filter inside of render_view.cc
>
>
>
http://codereview.chromium.**org/7193001/<http://codereview.chromium.org/7193...
>

[scherkus (not reviewing), 2011-06-24 00:23:20]

+acolwell to review + help answer ronghuawu's question

[wjia(left Chromium), 2011-06-24 01:33:38]

Ronghua,

This problem can be resolved without touching FilterCollection in
RenderView. A couple of helper functions can be added in StreamClient or a
new helper class to create RtcVideoDecoder/CaptureVideoDecoder. The
implementation of those 2 helper functions would be in
content/renderer/media, e.g., in MediaStreamImpl.

Wei

On Thu, Jun 23, 2011 at 5:23 PM, <scherkus@chromium.org> wrote:

> +acolwell to review + help answer ronghuawu's question
>
>
>
http://codereview.chromium.**org/7193001/<http://codereview.chromium.org/7193...
>

[Ronghua, 2011-06-24 15:27:54]

Thanks Wei! That's a great idea.

I'm going to do the moving first and remove the RtcVideoDecoder from
webmediaplayer_**impl for now and add back later when  MediaStream is ready.

Regards,
Ronghua

On Thu, Jun 23, 2011 at 6:33 PM, Wei Jia <wjia@chromium.org> wrote:

> Ronghua,
>
> This problem can be resolved without touching FilterCollection in
> RenderView. A couple of helper functions can be added in StreamClient or a
> new helper class to create RtcVideoDecoder/CaptureVideoDecoder. The
> implementation of those 2 helper functions would be in
> content/renderer/media, e.g., in MediaStreamImpl.
>
> Wei
>
>
> On Thu, Jun 23, 2011 at 5:23 PM, <scherkus@chromium.org> wrote:
>
>> +acolwell to review + help answer ronghuawu's question
>>
>>
>>
http://codereview.chromium.**org/7193001/<http://codereview.chromium.org/7193...
>>
>
>

[acolwell GONE FROM CHROMIUM, 2011-06-24 17:11:27]

Looks pretty good. Just adding to Andrew's comment about raw_video

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:622: if (raw_video) {
On 2011/06/17 00:31:46, scherkus wrote:
> this raw_video thing is kind of hacky....
> 
> can we perhaps inspect the scheme of the URL?
I agree. I think we could put a kRawVideoScheme string constant in pipeline.h
and use that to control this behavior. Then the code outside media/ can use it
to construct URLs to send to the pipeline.

[Ronghua, 2011-06-24 21:08:51]

* Removed the "namespace media".
* Removed the creation of RTCVideoDecoder from webmediaplayer_impl.cc. Will add
it back when we have the MediaStream ready.
* Added kRawVideoScheme to pipeline.h.
* Inspected the kRawVideoScheme in pipeline_impl.cc.

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.cc:18: namespace media {
On 2011/06/17 00:31:46, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder.h:29: namespace media {
On 2011/06/17 00:31:46, scherkus wrote:
> you're no longer inside media namespace

Done.

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/3004/content/renderer/media/rtc_v...
content/renderer/media/rtc_video_decoder_unittest.cc:32: namespace media {
On 2011/06/17 00:31:46, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:622: if (raw_video) {
On 2011/06/17 00:31:46, scherkus wrote:
> this raw_video thing is kind of hacky....
> 
> can we perhaps inspect the scheme of the URL?

Done.

http://codereview.chromium.org/7193001/diff/3004/media/base/pipeline_impl.cc#...
media/base/pipeline_impl.cc:622: if (raw_video) {
On 2011/06/24 17:11:27, acolwell wrote:
> On 2011/06/17 00:31:46, scherkus wrote:
> > this raw_video thing is kind of hacky....
> > 
> > can we perhaps inspect the scheme of the URL?
> I agree. I think we could put a kRawVideoScheme string constant in pipeline.h
> and use that to control this behavior. Then the code outside media/ can use it
> to construct URLs to send to the pipeline.

Done.

http://codereview.chromium.org/7193001/diff/3004/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/7193001/diff/3004/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:12: #include
"content/renderer/media/rtc_video_decoder.h"
On 2011/06/17 00:31:46, scherkus wrote:
> webkit glue can't depend on content
> 
> AFAIK you'll have to add this filter inside of render_view.cc

Done.

[scherkus (not reviewing), 2011-06-24 21:33:39]

few style stuff

thanks for making the change to inspect the URL!

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.cc:29: void
RTCVideoDecoder::Initialize(media::DemuxerStream* demuxer_stream,
if you want inside .cc files you may use:

using media::DemuxerStream;

etc... to save you some typing

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:18: class ExternalRenderer {
can you move this into a separate .h file?

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:29: class RTCVideoDecoder : public
media::VideoDecoder,
nit: use initializer list format

class RTCVideoDecoder
    : public media::VideoDecoder,
      public ExternalRenderer

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline.h#newco...
media/base/pipeline.h:24: static const char kRawVideoScheme[] = "media";
constants should be initialized inside .cc files

also is "media" really a good scheme name?

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:17: #include "googleurl/src/gurl.h"
media shouldn't depend on googleurl

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:621: if (gurl.SchemeIs(kRawVideoScheme)) {
hmm... perhaps we can just do a check using std::string functions

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl_un...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:37: static const std::string kUrlMedia =
std::string(kRawVideoScheme) +
this really shouldn't be a std::string (we only use basic data types for static
constants)

instead please use char[]

[acolwell GONE FROM CHROMIUM, 2011-06-24 22:53:43]

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline.h#newco...
media/base/pipeline.h:24: static const char kRawVideoScheme[] = "media";
On 2011/06/24 21:33:39, scherkus wrote:
> constants should be initialized inside .cc files
> 
> also is "media" really a good scheme name?
You should use an x- prefix for a private scheme. Something like "x-raw-video"
might be better.

[Ronghua, 2011-06-27 22:34:51]

* Used "x-raw-media" as the scheme as it will be used on video and audio.
* Replaced the gurl.SchemeIs with str compare.
* Implemented RTCVideoDecoder as cricket::VideoRenderer, which is the libjingle
renderer interface.

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.cc:29: void
RTCVideoDecoder::Initialize(media::DemuxerStream* demuxer_stream,
On 2011/06/24 21:33:39, scherkus wrote:
> if you want inside .cc files you may use:
> 
> using media::DemuxerStream;
> 
> etc... to save you some typing

Done.

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:29: class RTCVideoDecoder : public
media::VideoDecoder,
On 2011/06/24 21:33:39, scherkus wrote:
> nit: use initializer list format
> 
> class RTCVideoDecoder
>     : public media::VideoDecoder,
>       public ExternalRenderer

Done.

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline.h#newco...
media/base/pipeline.h:24: static const char kRawVideoScheme[] = "media";
On 2011/06/24 22:53:43, acolwell wrote:
> On 2011/06/24 21:33:39, scherkus wrote:
> > constants should be initialized inside .cc files
> > 
> > also is "media" really a good scheme name?
> You should use an x- prefix for a private scheme. Something like "x-raw-video"
> might be better.

Done.

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:17: #include "googleurl/src/gurl.h"
On 2011/06/24 21:33:39, scherkus wrote:
> media shouldn't depend on googleurl

Done.

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:621: if (gurl.SchemeIs(kRawVideoScheme)) {
On 2011/06/24 21:33:39, scherkus wrote:
> hmm... perhaps we can just do a check using std::string functions

Done.

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl_un...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:37: static const std::string kUrlMedia =
std::string(kRawVideoScheme) +
On 2011/06/24 21:33:39, scherkus wrote:
> this really shouldn't be a std::string (we only use basic data types for
static
> constants)
> 
> instead please use char[]

Done.

[scherkus (not reviewing), 2011-06-27 23:06:08]

http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:16: #include
"third_party/libjingle/source/talk/session/phone/mediachannel.h"
woah!?

why are we now implementing cricket::VideoRenderer?

that changes the original intent of this patch which was supposed to be a move

[Ronghua, 2011-06-28 01:41:22]

Hi Andrew,

The ExternalRenderer is an interface defined in a header file in
"third_party/webrtc". We defined it here because at that time the
webrtc was not yet in chromium.  Later we realized that we should
implement this as cricket::VideoRenderer, which is an interface
defined by libjingle.

You suggested at we should put the ExternalRenderer into a separated
header file, but I prefer not to do that because this ExternalRenderer
will soon be replaced by cricket::VideoRenderer from
"third_party/libjingle/source/talk/session/phone/mediachannel.h". That
is why I though maybe I can just do it in this patch. However, you are
right, we should keep this patch as a move. Do you think it's ok if I
keep the ExternalRenderer as it's (not move it to a separated header),
since it will be replaced by "cricket::VideoRenderer" right after this
patch.

Thanks,
Ronghua

On Mon, Jun 27, 2011 at 4:06 PM, <scherkus@chromium.org> wrote:
>
>
http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_...
> File content/renderer/media/rtc_video_decoder.h (right):
>
>
http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.h:16: #include
> "third_party/libjingle/source/talk/session/phone/mediachannel.h"
> woah!?
>
> why are we now implementing cricket::VideoRenderer?
>
> that changes the original intent of this patch which was supposed to be
> a move
>
> http://codereview.chromium.org/7193001/

[scherkus (not reviewing), 2011-06-28 20:26:14]

Let's revert the cricket header inclusion + interface for now and move the
file as originally intended.

I'm not fully convinced we should start depending on libjingle code just yet
so I'd prefer to keep code landing as it should be instead of temporarily
deviating from the style guide.

Andrew

On Mon, Jun 27, 2011 at 6:40 PM, Ronghua Wu <ronghuawu@google.com> wrote:

> Hi Andrew,
>
> The ExternalRenderer is an interface defined in a header file in
> "third_party/webrtc". We defined it here because at that time the
> webrtc was not yet in chromium.  Later we realized that we should
> implement this as cricket::VideoRenderer, which is an interface
> defined by libjingle.
>
> You suggested at we should put the ExternalRenderer into a separated
> header file, but I prefer not to do that because this ExternalRenderer
> will soon be replaced by cricket::VideoRenderer from
> "third_party/libjingle/source/talk/session/phone/mediachannel.h". That
> is why I though maybe I can just do it in this patch. However, you are
> right, we should keep this patch as a move. Do you think it's ok if I
> keep the ExternalRenderer as it's (not move it to a separated header),
> since it will be replaced by "cricket::VideoRenderer" right after this
> patch.
>
> Thanks,
> Ronghua
>
> On Mon, Jun 27, 2011 at 4:06 PM, <scherkus@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_...
> > File content/renderer/media/rtc_video_decoder.h (right):
> >
> >
>
http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_...
> > content/renderer/media/rtc_video_decoder.h:16: #include
> > "third_party/libjingle/source/talk/session/phone/mediachannel.h"
> > woah!?
> >
> > why are we now implementing cricket::VideoRenderer?
> >
> > that changes the original intent of this patch which was supposed to be
> > a move
> >
> > http://codereview.chromium.org/7193001/
>

[Ronghua, 2011-06-28 22:58:46]

The ExternalRenderer is spited into a new file "webrtc_external_renderer.h" as
suggested by Andrew.

[scherkus (not reviewing), 2011-06-29 00:39:27]

thanks for making the change

few more simple nits and we'll be good to go

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.cc:63:
static_cast<int>(VideoFrame::TYPE_SYSTEM_MEMORY));
why was this changed?

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.cc:123: const FilterStatusCB& cb) {
why was this dropped to next line?

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:40: virtual int
FrameSizeChange(unsigned int width,
instead of "unsigned int" use either "int" or if you really want unsigned
"size_t"

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:44: virtual int DeliverFrame(unsigned
char* buffer,
"unsigned char" -> uint8

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:49: static bool IsUrlSupported(const
std::string& url);
this is no longer defined in the .cc

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:80: 
nit: could you get rid of blank line here

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder_unittest.cc:145: unsigned int
video_frame_size = decoder_->width_*decoder_->height_*3/2;
spaces around binary operators (*, /, etc)

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder_unittest.cc:146: unsigned char*
video_frame = new unsigned char[video_frame_size];
uint8

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder_unittest.cc:179: 
nit: blank line

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
File content/renderer/media/webrtc_external_renderer.h (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
content/renderer/media/webrtc_external_renderer.h:10: class ExternalRenderer {
filename should match class name, so either:
  1) Rename class to WebRTCExternalRenderer
  2) Rename file to external_renderer.h

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
content/renderer/media/webrtc_external_renderer.h:22: 
nit: blank line

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline.h#newco...
media/base/pipeline.h:24: static const char kRawMediaScheme[] = "x-raw-media";
please initialize this in the .cc

.h should have:
extern const char kRawMediaScheme[];

.cc should have:
const char kRawMediaScheme[] = "x-raw-media";

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:619: bool raw_media = true;
I believe you can replace all of this with a helper from base/string_util.h

base::strncasecmp() should do the trick

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:37: static const char kUrlRawVideo[] =
"://raw_video_stream";
why not just append the x-raw-video: scheme here and avoid the append?

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:210: run_build = false;
this is over-indented -- can you fix?

[scherkus (not reviewing), 2011-06-29 17:28:05]

http://codereview.chromium.org/7193001/diff/24008/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7193001/diff/24008/media/media.gyp#newcode19
media/media.gyp:19: '../build/temp_gyp/googleurl.gyp:googleurl',
just noticed... please remove this dependency since it was added when you
initially added the rtc filter

[Ronghua, 2011-06-29 20:36:39]

Updated according to the comments.

The only thing I didn't change is the definition of ExternalRenderer, which
comes from WebRTC project. So I can't change it here.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.cc (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.cc:63:
static_cast<int>(VideoFrame::TYPE_SYSTEM_MEMORY));
On 2011/06/29 00:39:28, scherkus wrote:
> why was this changed?

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.cc:123: const FilterStatusCB& cb) {
On 2011/06/29 00:39:28, scherkus wrote:
> why was this dropped to next line?

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder.h (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:40: virtual int
FrameSizeChange(unsigned int width,
On 2011/06/29 00:39:28, scherkus wrote:
> instead of "unsigned int" use either "int" or if you really want unsigned
> "size_t"

The ExternalRenderer interface is defined in WebRTC project, so I can't change
it here. Plus this is a moving, how about keep it as it was?

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:44: virtual int DeliverFrame(unsigned
char* buffer,
On 2011/06/29 00:39:28, scherkus wrote:
> "unsigned char" -> uint8

The ExternalRenderer interface is defined in WebRTC project, so I can't change
it here.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:49: static bool IsUrlSupported(const
std::string& url);
On 2011/06/29 00:39:28, scherkus wrote:
> this is no longer defined in the .cc

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder.h:80: 
On 2011/06/29 00:39:28, scherkus wrote:
> nit: could you get rid of blank line here

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder_unittest.cc:145: unsigned int
video_frame_size = decoder_->width_*decoder_->height_*3/2;
On 2011/06/29 00:39:28, scherkus wrote:
> spaces around binary operators (*, /, etc)

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder_unittest.cc:146: unsigned char*
video_frame = new unsigned char[video_frame_size];
On 2011/06/29 00:39:28, scherkus wrote:
> uint8

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
content/renderer/media/rtc_video_decoder_unittest.cc:179: 
On 2011/06/29 00:39:28, scherkus wrote:
> nit: blank line

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
File content/renderer/media/webrtc_external_renderer.h (right):

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
content/renderer/media/webrtc_external_renderer.h:10: class ExternalRenderer {
On 2011/06/29 00:39:28, scherkus wrote:
> filename should match class name, so either:
>   1) Rename class to WebRTCExternalRenderer
>   2) Rename file to external_renderer.h

Done.

http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
content/renderer/media/webrtc_external_renderer.h:22: 
On 2011/06/29 00:39:28, scherkus wrote:
> nit: blank line

Done.

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline.h#newco...
media/base/pipeline.h:24: static const char kRawMediaScheme[] = "x-raw-media";
On 2011/06/29 00:39:28, scherkus wrote:
> please initialize this in the .cc
> 
> .h should have:
> extern const char kRawMediaScheme[];
> 
> .cc should have:
> const char kRawMediaScheme[] = "x-raw-media";

Done.

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:619: bool raw_media = true;
On 2011/06/29 00:39:28, scherkus wrote:
> I believe you can replace all of this with a helper from base/string_util.h
> 
> base::strncasecmp() should do the trick

Done.

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:37: static const char kUrlRawVideo[] =
"://raw_video_stream";
On 2011/06/29 00:39:28, scherkus wrote:
> why not just append the x-raw-video: scheme here and avoid the append?

I don't want to hard code the "x-raw-media" here, but I don't know how to append
kRawMediaScheme to kUrlRawVideo here.

http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:210: run_build = false;
On 2011/06/29 00:39:28, scherkus wrote:
> this is over-indented -- can you fix?

Done.

http://codereview.chromium.org/7193001/diff/24008/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7193001/diff/24008/media/media.gyp#newcode19
media/media.gyp:19: '../build/temp_gyp/googleurl.gyp:googleurl',
On 2011/06/29 17:28:05, scherkus wrote:
> just noticed... please remove this dependency since it was added when you
> initially added the rtc filter

Done.

[Ronghua, 2011-07-06 17:37:33]

On 2011/06/29 20:36:39, ronghuawu wrote:
> Updated according to the comments.
> 
> The only thing I didn't change is the definition of ExternalRenderer, which
> comes from WebRTC project. So I can't change it here.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> File content/renderer/media/rtc_video_decoder.cc (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.cc:63:
> static_cast<int>(VideoFrame::TYPE_SYSTEM_MEMORY));
> On 2011/06/29 00:39:28, scherkus wrote:
> > why was this changed?
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.cc:123: const FilterStatusCB& cb) {
> On 2011/06/29 00:39:28, scherkus wrote:
> > why was this dropped to next line?
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> File content/renderer/media/rtc_video_decoder.h (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.h:40: virtual int
> FrameSizeChange(unsigned int width,
> On 2011/06/29 00:39:28, scherkus wrote:
> > instead of "unsigned int" use either "int" or if you really want unsigned
> > "size_t"
> 
> The ExternalRenderer interface is defined in WebRTC project, so I can't change
> it here. Plus this is a moving, how about keep it as it was?
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.h:44: virtual int
DeliverFrame(unsigned
> char* buffer,
> On 2011/06/29 00:39:28, scherkus wrote:
> > "unsigned char" -> uint8
> 
> The ExternalRenderer interface is defined in WebRTC project, so I can't change
> it here.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.h:49: static bool
IsUrlSupported(const
> std::string& url);
> On 2011/06/29 00:39:28, scherkus wrote:
> > this is no longer defined in the .cc
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder.h:80: 
> On 2011/06/29 00:39:28, scherkus wrote:
> > nit: could you get rid of blank line here
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> File content/renderer/media/rtc_video_decoder_unittest.cc (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder_unittest.cc:145: unsigned int
> video_frame_size = decoder_->width_*decoder_->height_*3/2;
> On 2011/06/29 00:39:28, scherkus wrote:
> > spaces around binary operators (*, /, etc)
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder_unittest.cc:146: unsigned char*
> video_frame = new unsigned char[video_frame_size];
> On 2011/06/29 00:39:28, scherkus wrote:
> > uint8
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_...
> content/renderer/media/rtc_video_decoder_unittest.cc:179: 
> On 2011/06/29 00:39:28, scherkus wrote:
> > nit: blank line
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
> File content/renderer/media/webrtc_external_renderer.h (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
> content/renderer/media/webrtc_external_renderer.h:10: class ExternalRenderer {
> On 2011/06/29 00:39:28, scherkus wrote:
> > filename should match class name, so either:
> >   1) Rename class to WebRTCExternalRenderer
> >   2) Rename file to external_renderer.h
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webr...
> content/renderer/media/webrtc_external_renderer.h:22: 
> On 2011/06/29 00:39:28, scherkus wrote:
> > nit: blank line
> 
> Done.
> 
> http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline.h
> File media/base/pipeline.h (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline.h#newco...
> media/base/pipeline.h:24: static const char kRawMediaScheme[] = "x-raw-media";
> On 2011/06/29 00:39:28, scherkus wrote:
> > please initialize this in the .cc
> > 
> > .h should have:
> > extern const char kRawMediaScheme[];
> > 
> > .cc should have:
> > const char kRawMediaScheme[] = "x-raw-media";
> 
> Done.
> 
> http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl.cc
> File media/base/pipeline_impl.cc (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl.cc...
> media/base/pipeline_impl.cc:619: bool raw_media = true;
> On 2011/06/29 00:39:28, scherkus wrote:
> > I believe you can replace all of this with a helper from base/string_util.h
> > 
> > base::strncasecmp() should do the trick
> 
> Done.
> 
>
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
> File media/base/pipeline_impl_unittest.cc (right):
> 
>
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
> media/base/pipeline_impl_unittest.cc:37: static const char kUrlRawVideo[] =
> "://raw_video_stream";
> On 2011/06/29 00:39:28, scherkus wrote:
> > why not just append the x-raw-video: scheme here and avoid the append?
> 
> I don't want to hard code the "x-raw-media" here, but I don't know how to
append
> kRawMediaScheme to kUrlRawVideo here.
> 
>
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_un...
> media/base/pipeline_impl_unittest.cc:210: run_build = false;
> On 2011/06/29 00:39:28, scherkus wrote:
> > this is over-indented -- can you fix?
> 
> Done.
> 
> http://codereview.chromium.org/7193001/diff/24008/media/media.gyp
> File media/media.gyp (right):
> 
> http://codereview.chromium.org/7193001/diff/24008/media/media.gyp#newcode19
> media/media.gyp:19: '../build/temp_gyp/googleurl.gyp:googleurl',
> On 2011/06/29 17:28:05, scherkus wrote:
> > just noticed... please remove this dependency since it was added when you
> > initially added the rtc filter
> 
> Done.

Ping, anything else needs to be done for this patch?

[acolwell GONE FROM CHROMIUM, 2011-07-06 17:50:31]

LGTM

[Ronghua, 2011-07-06 20:32:46]

Merged with the latest chromium. Will let Wei help try it.

[commit-bot: I haz the power, 2011-07-06 20:41:19]

Can't process patch for file content/renderer/media/rtc_video_decoder.cc.
A  +