http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_video_decoder.cc
File content/renderer/media/rtc_video_decoder.cc (right):
http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_video_decoder.cc#newcode29
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_video_decoder.h
File content/renderer/media/rtc_video_decoder.h (right):
http://codereview.chromium.org/7193001/diff/17003/content/renderer/media/rtc_video_decoder.h#newcode29
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#newcode24
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#newcode17
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#newcode621
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_unittest.cc
File media/base/pipeline_impl_unittest.cc (right):
http://codereview.chromium.org/7193001/diff/17003/media/base/pipeline_impl_unittest.cc#newcode37
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.
http://codereview.chromium.org/7193001/diff/22003/content/renderer/media/rtc_video_decoder.h#newcode16
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
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
few more simple nits and we'll be good to go
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder.cc
File content/renderer/media/rtc_video_decoder.cc (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder.cc#newcode63
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_video_decoder.cc#newcode123
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_video_decoder.h
File content/renderer/media/rtc_video_decoder.h (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder.h#newcode40
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_video_decoder.h#newcode44
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_video_decoder.h#newcode49
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_video_decoder.h#newcode80
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_video_decoder_unittest.cc
File content/renderer/media/rtc_video_decoder_unittest.cc (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder_unittest.cc#newcode145
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_video_decoder_unittest.cc#newcode146
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_video_decoder_unittest.cc#newcode179
content/renderer/media/rtc_video_decoder_unittest.cc:179:
nit: blank line
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webrtc_external_renderer.h
File content/renderer/media/webrtc_external_renderer.h (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webrtc_external_renderer.h#newcode10
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/webrtc_external_renderer.h#newcode22
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#newcode24
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#newcode619
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_unittest.cc
File media/base/pipeline_impl_unittest.cc (right):
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_unittest.cc#newcode37
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_unittest.cc#newcode210
media/base/pipeline_impl_unittest.cc:210: run_build = false;
this is over-indented -- can you fix?
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
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_video_decoder.cc
File content/renderer/media/rtc_video_decoder.cc (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder.cc#newcode63
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_video_decoder.cc#newcode123
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_video_decoder.h
File content/renderer/media/rtc_video_decoder.h (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder.h#newcode40
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_video_decoder.h#newcode44
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_video_decoder.h#newcode49
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_video_decoder.h#newcode80
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_video_decoder_unittest.cc
File content/renderer/media/rtc_video_decoder_unittest.cc (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/rtc_video_decoder_unittest.cc#newcode145
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_video_decoder_unittest.cc#newcode146
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_video_decoder_unittest.cc#newcode179
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/webrtc_external_renderer.h
File content/renderer/media/webrtc_external_renderer.h (right):
http://codereview.chromium.org/7193001/diff/24008/content/renderer/media/webrtc_external_renderer.h#newcode10
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/webrtc_external_renderer.h#newcode22
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#newcode24
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#newcode619
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_unittest.cc
File media/base/pipeline_impl_unittest.cc (right):
http://codereview.chromium.org/7193001/diff/24008/media/base/pipeline_impl_unittest.cc#newcode37
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_unittest.cc#newcode210
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.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
> Done.
Ping, anything else needs to be done for this patch?