Implement VP8 encoder for chromoting

Implement VP8 encoder for chromoting

Added EncoderVp8 with test for chromoting.

TEST=remoting_unittests
BUG=50235
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60854

[dmac, 2010-07-30 22:57:29]

http://codereview.chromium.org/3005036/diff/3001/4002
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/3005036/diff/3001/4002#newcode42
remoting/base/encoder_vp8.cc:42: image_->fmt = (vpx_img_fmt_t)(0x100 | 0x200 |
1);
are there no constants for these?

http://codereview.chromium.org/3005036/diff/3001/4002#newcode73
remoting/base/encoder_vp8.cc:73: const int size = plane_size * 3 / 2;
comment about the rounding here?

http://codereview.chromium.org/3005036/diff/3001/4002#newcode84
remoting/base/encoder_vp8.cc:84: plane_size / 4;
comments about why plane_size / 4?

http://codereview.chromium.org/3005036/diff/3001/4002#newcode86
remoting/base/encoder_vp8.cc:86: image_->stride[1] = image_->w / 2;
comment about why width / 2

http://codereview.chromium.org/3005036/diff/3001/4002#newcode99
remoting/base/encoder_vp8.cc:99: memcpy(out, in, width);
is there a conversion happening here?

[Alpha Left Google, 2010-07-30 23:26:37]

Thanks for review. Comments are addressed.

http://codereview.chromium.org/3005036/diff/3001/4002
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/3005036/diff/3001/4002#newcode42
remoting/base/encoder_vp8.cc:42: image_->fmt = (vpx_img_fmt_t)(0x100 | 0x200 |
1);
On 2010/07/30 22:57:30, dmac wrote:
> are there no constants for these?

Done.

http://codereview.chromium.org/3005036/diff/3001/4002#newcode84
remoting/base/encoder_vp8.cc:84: plane_size / 4;
On 2010/07/30 22:57:30, dmac wrote:
> comments about why plane_size / 4?

Done.

http://codereview.chromium.org/3005036/diff/3001/4002#newcode86
remoting/base/encoder_vp8.cc:86: image_->stride[1] = image_->w / 2;
On 2010/07/30 22:57:30, dmac wrote:
> comment about why width / 2

Done.

http://codereview.chromium.org/3005036/diff/3001/4002#newcode99
remoting/base/encoder_vp8.cc:99: memcpy(out, in, width);
Thanks. I just noticed the algorithm used here is incorrect. Changed to a proper
implementation.

[dmac, 2010-07-30 23:39:00]

On 2010/07/30 23:26:37, Alpha wrote:
> Thanks for review. Comments are addressed.
> 
> http://codereview.chromium.org/3005036/diff/3001/4002
> File remoting/base/encoder_vp8.cc (right):
> 
> http://codereview.chromium.org/3005036/diff/3001/4002#newcode42
> remoting/base/encoder_vp8.cc:42: image_->fmt = (vpx_img_fmt_t)(0x100 | 0x200 |
> 1);
> On 2010/07/30 22:57:30, dmac wrote:
> > are there no constants for these?
> 
> Done.
> 
> http://codereview.chromium.org/3005036/diff/3001/4002#newcode84
> remoting/base/encoder_vp8.cc:84: plane_size / 4;
> On 2010/07/30 22:57:30, dmac wrote:
> > comments about why plane_size / 4?
> 
> Done.
> 
> http://codereview.chromium.org/3005036/diff/3001/4002#newcode86
> remoting/base/encoder_vp8.cc:86: image_->stride[1] = image_->w / 2;
> On 2010/07/30 22:57:30, dmac wrote:
> > comment about why width / 2
> 
> Done.
> 
> http://codereview.chromium.org/3005036/diff/3001/4002#newcode99
> remoting/base/encoder_vp8.cc:99: memcpy(out, in, width);
> Thanks. I just noticed the algorithm used here is incorrect. Changed to a
proper
> implementation.

LGTM

[Alpha Left Google, 2010-08-02 21:20:15]

+scherkus to check the gyp changes for libvpx

[Alpha Left Google, 2010-08-31 23:19:11]

+ajwong

ajwong:
Please check the generated stub and media.cc

I'm not going to check this in as is as we have the following problems.
1. Windows' avcodec.dll doesn't have encode support. I'm waiting for this to be
in place before I implement GetVp8CxAlgoAddress() for windows. After that is
done it's ready to be committed.

2. This change will break Ubuntu since they build with system ffmpeg that
doesn't have vpx, definitely not encoder. In that case I need to link libvpx
directly but I'll do it in a separate patch.

[awong, 2010-08-31 23:39:57]

Wait...I'm confused.

If this isn't going to be checked in, then why do you want me to review it?

On Tue, Aug 31, 2010 at 4:19 PM, <hclam@chromium.org> wrote:

> +ajwong
>
> ajwong:
> Please check the generated stub and media.cc
>
> I'm not going to check this in as is as we have the following problems.
> 1. Windows' avcodec.dll doesn't have encode support. I'm waiting for this
> to be
> in place before I implement GetVp8CxAlgoAddress() for windows. After that
> is
> done it's ready to be committed.
>
> 2. This change will break Ubuntu since they build with system ffmpeg that
> doesn't have vpx, definitely not encoder. In that case I need to link
> libvpx
> directly but I'll do it in a separate patch.
>
>
> http://codereview.chromium.org/3005036/show
>

[Alpha Left Google, 2010-09-01 00:11:02]

Nevermind I have added code for windows too. Please take a look.

I'll fix the ubuntu problem later.

[awong, 2010-09-01 00:30:07]

Looks pretty solid.  Got some style nits.

http://codereview.chromium.org/3005036/diff/21001/22002
File media/base/media.h (right):

http://codereview.chromium.org/3005036/diff/21001/22002#newcode29
media/base/media.h:29: // This method should only be called after media library
is loaded.
put a newline above the TODO.  Makes it easier to see.

http://codereview.chromium.org/3005036/diff/21001/22002#newcode31
media/base/media.h:31: // purpose in libvpx.
Please file a bug to track too, and link to it from here.

http://codereview.chromium.org/3005036/diff/21001/22003
File media/base/media_posix.cc (right):

http://codereview.chromium.org/3005036/diff/21001/22003#newcode74
media/base/media_posix.cc:74: static void* vp8_cx_algo_address = NULL;
void* isn't the right type is it?

http://codereview.chromium.org/3005036/diff/21001/22006
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/3005036/diff/21001/22006#newcode139
remoting/base/encoder_vp8.cc:139: CHECK(ret == VPX_CODEC_OK) << "Encoding error:
"
Do we really want to CHECK fail on everything?

http://codereview.chromium.org/3005036/diff/21001/22006#newcode154
remoting/base/encoder_vp8.cc:154: // TODO(hclam): Make sure we get exactly one
packet encoding one frame.
do you mean one packet when encoding one frame?  It's unclear..

http://codereview.chromium.org/3005036/diff/21001/22006#newcode155
remoting/base/encoder_vp8.cc:155: // TODO(hclam): We should provide the output
buffer to save one copy.
save one -> avoid one.

http://codereview.chromium.org/3005036/diff/21001/22011
File remoting/run_all_unittests.cc (right):

http://codereview.chromium.org/3005036/diff/21001/22011#newcode5
remoting/run_all_unittests.cc:5: #include "base/at_exit.h"
Why is at_exit.h included?

http://codereview.chromium.org/3005036/diff/21001/22013
File tools/generate_stubs/generate_stubs.py (right):

http://codereview.chromium.org/3005036/diff/21001/22013#newcode47
tools/generate_stubs/generate_stubs.py:47: # Regular expression used to parse
functions signatures in the input files.
functions -> function

http://codereview.chromium.org/3005036/diff/21001/22013#newcode97
tools/generate_stubs/generate_stubs.py:97: va_list ___args;
don't use __ as a prefix to variable names.  Those identifiers are reserved for
the C++ compiler & standard library implementation.

Pick another unlikely name.

http://codereview.chromium.org/3005036/diff/21001/22013#newcode606
tools/generate_stubs/generate_stubs.py:606: return
RETURN_VARIADIC_STUB_FUNCTION_DEFINITION % {
Call this VARIDIC _STUB_FUNCTION_DEFINITION and
VOID_VARIADIC_STUB_FUNCTION_DEFINITION

http://codereview.chromium.org/3005036/diff/21001/22013#newcode619
tools/generate_stubs/generate_stubs.py:619: # This is a regular function.
Put this in an else block for the top level if.

[awong, 2010-09-01 00:31:43]

This change does not braek ubuntu does it?  The code will just init the
algorithm pointer to NULL, which will only cause a crash on the chromoting
encoder right?

-Albert

On Tue, Aug 31, 2010 at 5:11 PM, <hclam@chromium.org> wrote:

> Nevermind I have added code for windows too. Please take a look.
>
> I'll fix the ubuntu problem later.
>
>
>
>
> http://codereview.chromium.org/3005036/show
>

[Alpha Left Google, 2010-09-01 01:07:09]

Not really, InitializeMediaLibrary() will fail because the symbols for vp8
encoder are null.

[fbarchard, 2010-09-01 01:11:43]

Just got the libvpx drop that fixes lucid, so I'm building now with the
encoder.
Will just put dll's on filer
I dont want to check it into chromium until the Chrome7 branch is done.
Estimate 90 minutes

On Tue, Aug 31, 2010 at 6:07 PM, <hclam@chromium.org> wrote:

> Not really, InitializeMediaLibrary() will fail because the symbols for vp8
> encoder are null.
>
>
>
>
> http://codereview.chromium.org/3005036/show
>

[Alpha Left Google, 2010-09-01 22:35:28]

http://codereview.chromium.org/3005036/diff/21001/22002
File media/base/media.h (right):

http://codereview.chromium.org/3005036/diff/21001/22002#newcode29
media/base/media.h:29: // This method should only be called after media library
is loaded.
On 2010/09/01 00:30:07, awong wrote:
> put a newline above the TODO.  Makes it easier to see.

Done.

http://codereview.chromium.org/3005036/diff/21001/22002#newcode31
media/base/media.h:31: // purpose in libvpx.
On 2010/09/01 00:30:07, awong wrote:
> Please file a bug to track too, and link to it from here.

Done.

http://codereview.chromium.org/3005036/diff/21001/22003
File media/base/media_posix.cc (right):

http://codereview.chromium.org/3005036/diff/21001/22003#newcode74
media/base/media_posix.cc:74: static void* vp8_cx_algo_address = NULL;
On 2010/09/01 00:30:07, awong wrote:
> void* isn't the right type is it?

I want to avoid include vp8 headers from here.

http://codereview.chromium.org/3005036/diff/21001/22006
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/3005036/diff/21001/22006#newcode139
remoting/base/encoder_vp8.cc:139: CHECK(ret == VPX_CODEC_OK) << "Encoding error:
"
On 2010/09/01 00:30:07, awong wrote:
> Do we really want to CHECK fail on everything?

Done.

http://codereview.chromium.org/3005036/diff/21001/22006#newcode154
remoting/base/encoder_vp8.cc:154: // TODO(hclam): Make sure we get exactly one
packet encoding one frame.
On 2010/09/01 00:30:07, awong wrote:
> do you mean one packet when encoding one frame?  It's unclear..

Done.

http://codereview.chromium.org/3005036/diff/21001/22006#newcode155
remoting/base/encoder_vp8.cc:155: // TODO(hclam): We should provide the output
buffer to save one copy.
On 2010/09/01 00:30:07, awong wrote:
> save one -> avoid one.

Done.

http://codereview.chromium.org/3005036/diff/21001/22011
File remoting/run_all_unittests.cc (right):

http://codereview.chromium.org/3005036/diff/21001/22011#newcode5
remoting/run_all_unittests.cc:5: #include "base/at_exit.h"
On 2010/09/01 00:30:07, awong wrote:
> Why is at_exit.h included?

Done.

http://codereview.chromium.org/3005036/diff/21001/22013
File tools/generate_stubs/generate_stubs.py (right):

http://codereview.chromium.org/3005036/diff/21001/22013#newcode47
tools/generate_stubs/generate_stubs.py:47: # Regular expression used to parse
functions signatures in the input files.
On 2010/09/01 00:30:07, awong wrote:
> functions -> function

Done.

http://codereview.chromium.org/3005036/diff/21001/22013#newcode97
tools/generate_stubs/generate_stubs.py:97: va_list ___args;
On 2010/09/01 00:30:07, awong wrote:
> don't use __ as a prefix to variable names.  Those identifiers are reserved
for
> the C++ compiler & standard library implementation.
> 
> Pick another unlikely name.

Done.

http://codereview.chromium.org/3005036/diff/21001/22013#newcode606
tools/generate_stubs/generate_stubs.py:606: return
RETURN_VARIADIC_STUB_FUNCTION_DEFINITION % {
On 2010/09/01 00:30:07, awong wrote:
> Call this VARIDIC _STUB_FUNCTION_DEFINITION and
> VOID_VARIADIC_STUB_FUNCTION_DEFINITION

Done.

http://codereview.chromium.org/3005036/diff/21001/22013#newcode619
tools/generate_stubs/generate_stubs.py:619: # This is a regular function.
On 2010/09/01 00:30:07, awong wrote:
> Put this in an else block for the top level if.

Done.

[scherkus (not reviewing), 2010-09-07 19:53:41]

did a quick pass -- looks good but deferring to others for final LGTM

[awong, 2010-09-08 17:39:40]

Couple of comments.

http://codereview.chromium.org/3005036/diff/39001/40006
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/3005036/diff/39001/40006#newcode34
remoting/base/encoder_vp8.cc:34: delete codec_;
scoped_ptr?

http://codereview.chromium.org/3005036/diff/39001/40006#newcode43
remoting/base/encoder_vp8.cc:43: image_->fmt = VPX_IMG_FMT_YV12;
Can we avoid chained assignments?

Also, please add a comment saying what d_w/d_h versus w/h are.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode49
remoting/base/encoder_vp8.cc:49: (const
vpx_codec_iface_t*)media::GetVp8CxAlgoAddress();
C-style casts are disallowed.  Also, can't GetVp8CxAlgoAddress just return the
right type?

http://codereview.chromium.org/3005036/diff/39001/40006#newcode54
remoting/base/encoder_vp8.cc:54: // TODO(hclam): Adjust the parameters.
Adjust the parameters for what? Please elaborate a bit.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode86
remoting/base/encoder_vp8.cc:86: image_->planes[0] = (unsigned
char*)yuv_image_.get();
Alpha! Getting lazy!  Please fix all c-style casts!

http://codereview.chromium.org/3005036/diff/39001/40006#newcode109
remoting/base/encoder_vp8.cc:109: const int out_stride = image_->stride[0];
Isn't there code in media that we can share to do this?

http://codereview.chromium.org/3005036/diff/39001/40006#newcode122
remoting/base/encoder_vp8.cc:122: void
EncoderVp8::Encode(scoped_refptr<CaptureData> capture_data,
Just rethinking this...is it okay to pass in a scoped_refptr by copy?  What
exactly is the lifetime semantic of this supposed to be?  Can we use  a raw
pointer and define the API such that Encode never has to worry that capture_data
will be deletd out from under it without using a scoped_refptr?

[Alpha Left Google, 2010-09-08 20:32:20]

http://codereview.chromium.org/3005036/diff/39001/40006
File remoting/base/encoder_vp8.cc (right):

http://codereview.chromium.org/3005036/diff/39001/40006#newcode34
remoting/base/encoder_vp8.cc:34: delete codec_;
On 2010/09/08 17:39:40, awong wrote:
> scoped_ptr?

Done.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode43
remoting/base/encoder_vp8.cc:43: image_->fmt = VPX_IMG_FMT_YV12;
On 2010/09/08 17:39:40, awong wrote:
> Can we avoid chained assignments?
> 
> Also, please add a comment saying what d_w/d_h versus w/h are.

Done.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode49
remoting/base/encoder_vp8.cc:49: (const
vpx_codec_iface_t*)media::GetVp8CxAlgoAddress();
On 2010/09/08 17:39:40, awong wrote:
> C-style casts are disallowed.  Also, can't GetVp8CxAlgoAddress just return the
> right type?

ahhh.. I hate reinterpret_cast... :(

I don't want media.h to include vp8.h so i make it a void.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode54
remoting/base/encoder_vp8.cc:54: // TODO(hclam): Adjust the parameters.
On 2010/09/08 17:39:40, awong wrote:
> Adjust the parameters for what? Please elaborate a bit.

Done.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode86
remoting/base/encoder_vp8.cc:86: image_->planes[0] = (unsigned
char*)yuv_image_.get();
On 2010/09/08 17:39:40, awong wrote:
> Alpha! Getting lazy!  Please fix all c-style casts!

man I really hate reinterpret_cast, so many r's e's and t's.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode109
remoting/base/encoder_vp8.cc:109: const int out_stride = image_->stride[0];
On 2010/09/08 17:39:40, awong wrote:
> Isn't there code in media that we can share to do this?

Not for this purpose, this is an ad hoc solution. Need to revise this later.

http://codereview.chromium.org/3005036/diff/39001/40006#newcode122
remoting/base/encoder_vp8.cc:122: void
EncoderVp8::Encode(scoped_refptr<CaptureData> capture_data,
On 2010/09/08 17:39:40, awong wrote:
> Just rethinking this...is it okay to pass in a scoped_refptr by copy?  What
> exactly is the lifetime semantic of this supposed to be?  Can we use  a raw
> pointer and define the API such that Encode never has to worry that
capture_data
> will be deletd out from under it without using a scoped_refptr?

yes you can use a raw pointer. It is useful to be a scoped_refptr if you post a
task to Encode(), but that's not what we are doing.

[awong, 2010-09-08 20:36:56]

LGTM

[Alpha Left Google, 2010-09-08 20:37:58]

On 2010/09/08 20:36:56, awong wrote:
> LGTM

Too bad this probably has to stall for another 1 - 2 weeks since we are waiting
for the branch cut to include encoder to ffmpeg.

[scherkus (not reviewing), 2010-09-08 20:42:11]

On 2010/09/08 20:37:58, Alpha wrote:
> On 2010/09/08 20:36:56, awong wrote:
> > LGTM
> 
> Too bad this probably has to stall for another 1 - 2 weeks since we are
waiting
> for the branch cut to include encoder to ffmpeg.

Branch was cut, see:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

[hclam, 2010-09-08 20:46:55]

Anothony suggested to wait a bit. He said he'll know by today if 517 is ok.

Alpha

2010年9月8日13:42 <scherkus@chromium.org>:

> On 2010/09/08 20:37:58, Alpha wrote:
>
>> On 2010/09/08 20:36:56, awong wrote:
>> > LGTM
>>
>
>  Too bad this probably has to stall for another 1 - 2 weeks since we are
>>
> waiting
>
>> for the branch cut to include encoder to ffmpeg.
>>
>
> Branch was cut, see:
>
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
>
>
> http://codereview.chromium.org/3005036/show
>

[scherkus (not reviewing), 2010-09-08 20:47:57]

Ahh ok

Good to know!

On 2010/09/08 20:46:55, hclam wrote:
> Anothony suggested to wait a bit. He said he'll know by today if 517 is ok.
> 
> Alpha
> 
> 2010年9月8日13:42 <scherkus@chromium.org>:
> 
> > On 2010/09/08 20:37:58, Alpha wrote:
> >
> >> On 2010/09/08 20:36:56, awong wrote:
> >> > LGTM
> >>
> >
> >  Too bad this probably has to stall for another 1 - 2 weeks since we are
> >>
> > waiting
> >
> >> for the branch cut to include encoder to ffmpeg.
> >>
> >
> > Branch was cut, see:
> >
> >
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> >
> >
> > http://codereview.chromium.org/3005036/show
> >
>