Re: Move rtc_video_decoder* from media/filter/ to content/renderer/media/. (issue7193001)

0 views
Skip to first unread message

rong...@google.com

unread,
Jun 27, 2011, 6:34:51 PM6/27/11
to wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
* 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_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/

sche...@chromium.org

unread,
Jun 27, 2011, 7:06:08 PM6/27/11
to rong...@google.com, wj...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org

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

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

http://codereview.chromium.org/7193001/

Ronghua Wu

unread,
Jun 27, 2011, 9:40:55 PM6/27/11
to rong...@google.com, wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
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

Andrew Scherkus

unread,
Jun 28, 2011, 4:26:05 PM6/28/11
to Ronghua Wu, wj...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
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

rong...@google.com

unread,
Jun 28, 2011, 6:58:46 PM6/28/11
to wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
The ExternalRenderer is spited into a new file "webrtc_external_renderer.h"
as
suggested by Andrew.


http://codereview.chromium.org/7193001/

sche...@chromium.org

unread,
Jun 28, 2011, 8:39:27 PM6/28/11
to rong...@google.com, wj...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
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_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/

sche...@chromium.org

unread,
Jun 29, 2011, 1:28:05 PM6/29/11
to rong...@google.com, wj...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org

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

http://codereview.chromium.org/7193001/

rong...@google.com

unread,
Jun 29, 2011, 4:36:39 PM6/29/11
to wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
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_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.

http://codereview.chromium.org/7193001/

rong...@google.com

unread,
Jul 6, 2011, 1:37:33 PM7/6/11
to wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

> Done.

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

http://codereview.chromium.org/7193001/

acol...@chromium.org

unread,
Jul 6, 2011, 1:50:31 PM7/6/11
to rong...@google.com, wj...@chromium.org, sche...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
LGTM

http://codereview.chromium.org/7193001/

rong...@google.com

unread,
Jul 6, 2011, 4:32:46 PM7/6/11
to wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
Merged with the latest chromium. Will let Wei help try it.

http://codereview.chromium.org/7193001/

commi...@chromium.org

unread,
Jul 6, 2011, 4:41:19 PM7/6/11
to rong...@google.com, wj...@chromium.org, sche...@chromium.org, acol...@chromium.org, chromium...@chromium.org, hclam...@chromium.org, s...@chromium.org, ddorwi...@chromium.org, fischma...@chromium.org, phajd...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, acol...@chromium.org, ann...@chromium.org, dari...@chromium.org, ajwong...@chromium.org, v...@chromium.org
Can't process patch for file content/renderer/media/rtc_video_decoder.cc.
A +

http://codereview.chromium.org/7193001/

Reply all
Reply to author
Forward
0 new messages