Simplify VideoEncoder interface.

Simplify VideoEncoder interface.

This should also avoid assert in the linked bug.

BUG=284775
R=wez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223152

[Wez, 2013-09-11 07:34:16]

LGTM, but doesn't the Verbatim encoder also need updating?

[Sergey Ulanov, 2013-09-11 07:50:05]

On 2013/09/11 07:34:16, Wez wrote:
> LGTM, but doesn't the Verbatim encoder also need updating?

That's why I wanted to remove it - verbatim codec sends multiple VideoPackets
per frame, so the new interface wouldn't work for it.

[Jamie, 2013-09-11 15:08:08]

On 2013/09/11 07:50:05, Sergey Ulanov wrote:
> On 2013/09/11 07:34:16, Wez wrote:
> > LGTM, but doesn't the Verbatim encoder also need updating?
> 
> That's why I wanted to remove it - verbatim codec sends multiple VideoPackets
> per frame, so the new interface wouldn't work for it.

How much work would it be to update it? Even if we're not using it in
production, a simple baseline codec seems like it might be useful, perhaps for
testing. Not so much so that it's worth putting a lot of effort into maintaining
it, though.

[Sergey Ulanov, 2013-09-11 19:00:28]

On 2013/09/11 15:08:08, Jamie wrote:
> On 2013/09/11 07:50:05, Sergey Ulanov wrote:
> > On 2013/09/11 07:34:16, Wez wrote:
> > > LGTM, but doesn't the Verbatim encoder also need updating?
> > 
> > That's why I wanted to remove it - verbatim codec sends multiple
VideoPackets
> > per frame, so the new interface wouldn't work for it.
> 
> How much work would it be to update it? Even if we're not using it in
> production, a simple baseline codec seems like it might be useful, perhaps for
> testing. Not so much so that it's worth putting a lot of effort into
maintaining
> it, though.

It shouldn't be hard to modify it if we don't need to keep it backwards
compatible, but that would essentially require reimplementing verbatim codec - I
think it can be done later when we have a use case for it.

[Jamie, 2013-09-11 19:26:31]

On 2013/09/11 19:00:28, Sergey Ulanov wrote:
> On 2013/09/11 15:08:08, Jamie wrote:
> > On 2013/09/11 07:50:05, Sergey Ulanov wrote:
> > > On 2013/09/11 07:34:16, Wez wrote:
> > > > LGTM, but doesn't the Verbatim encoder also need updating?
> > > 
> > > That's why I wanted to remove it - verbatim codec sends multiple
> VideoPackets
> > > per frame, so the new interface wouldn't work for it.
> > 
> > How much work would it be to update it? Even if we're not using it in
> > production, a simple baseline codec seems like it might be useful, perhaps
for
> > testing. Not so much so that it's worth putting a lot of effort into
> maintaining
> > it, though.
> 
> It shouldn't be hard to modify it if we don't need to keep it backwards
> compatible, but that would essentially require reimplementing verbatim codec -
I
> think it can be done later when we have a use case for it.

If fixing it now isn't going to save us any time in the future, then I agree it
can go for now.

[Sergey Ulanov, 2013-09-11 22:07:47]

I've rewritten verbatim video codec implementation. PTAL.

[Wez, 2013-09-12 14:20:15]

https://codereview.chromium.org/23477059/diff/12001/remoting/client/rectangle...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/client/rectangle...
remoting/client/rectangle_update_decoder.cc:47: if (codec ==
ChannelConfig::CODEC_VP8) {
Need to restore the CODEC_VERBATIM case here.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/audio_deco...
File remoting/codec/audio_decoder.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/audio_deco...
remoting/codec/audio_decoder.cc:18: if (audio_config.codec ==
protocol::ChannelConfig::CODEC_OPUS) {
Need to restore the CODEC_VERBATIM case here, so we can do Verbatim tests to
standard clients.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
File remoting/codec/video_decoder_verbatim.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
remoting/codec/video_decoder_verbatim.cc:53: int stride = kBytesPerPixel *
screen_size_.width();
nit: buffer_row_bytes or buffer_stride?

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
remoting/codec/video_decoder_verbatim.cc:55: const SkIRect& rect = iter.rect();
Need to check that |rect| is contained within |screen_size_| here.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
remoting/codec/video_decoder_verbatim.cc:56: int row_size = kBytesPerPixel *
rect.width();
nit: row_bytes or source_stride or packet[_data]_stride?

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:46: const int stride = frame.stride();
nit: in_stride?

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:52: const int row_size =
webrtc::DesktopFrame::kBytesPerPixel * rect.width();
nit: row_bytes or out_stride?

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:76: packet->set_flags(packet->flags() |
VideoPacket::LAST_PARTITION);
nit: Merge this into the earlier set_flags() call.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim.h (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.h:30: // The most recent screen size.
nit: Suggest "Used to re-send screen-size only when it changes."

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim_unittest.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim_unittest.cc:15:
TestVideoEncoder(encoder.get(), false);
Why not strict?

[Sergey Ulanov, 2013-09-12 19:18:45]

cleaned up video.proto too.

https://codereview.chromium.org/23477059/diff/12001/remoting/client/rectangle...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/client/rectangle...
remoting/client/rectangle_update_decoder.cc:47: if (codec ==
ChannelConfig::CODEC_VP8) {
On 2013/09/12 14:20:15, Wez wrote:
> Need to restore the CODEC_VERBATIM case here.

Done.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/audio_deco...
File remoting/codec/audio_decoder.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/audio_deco...
remoting/codec/audio_decoder.cc:18: if (audio_config.codec ==
protocol::ChannelConfig::CODEC_OPUS) {
On 2013/09/12 14:20:15, Wez wrote:
> Need to restore the CODEC_VERBATIM case here, so we can do Verbatim tests to
> standard clients.

Done.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
File remoting/codec/video_decoder_verbatim.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
remoting/codec/video_decoder_verbatim.cc:53: int stride = kBytesPerPixel *
screen_size_.width();
On 2013/09/12 14:20:15, Wez wrote:
> nit: buffer_row_bytes or buffer_stride?

They are the same for the frames generated by this decoder.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
remoting/codec/video_decoder_verbatim.cc:55: const SkIRect& rect = iter.rect();
On 2013/09/12 14:20:15, Wez wrote:
> Need to check that |rect| is contained within |screen_size_| here.

Done.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_deco...
remoting/codec/video_decoder_verbatim.cc:56: int row_size = kBytesPerPixel *
rect.width();
On 2013/09/12 14:20:15, Wez wrote:
> nit: row_bytes or source_stride or packet[_data]_stride?

size of a single raw in the rect being decoded.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:46: const int stride = frame.stride();
On 2013/09/12 14:20:15, Wez wrote:
> nit: in_stride?

Done.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:52: const int row_size =
webrtc::DesktopFrame::kBytesPerPixel * rect.width();
On 2013/09/12 14:20:15, Wez wrote:
> nit: row_bytes or out_stride?

there is no out_stride, because it's not a single image - instead it's a
sequence of rows of updated rectangles. This is is size of a single row for in
the current rectangle.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:76: packet->set_flags(packet->flags() |
VideoPacket::LAST_PARTITION);
On 2013/09/12 14:20:15, Wez wrote:
> nit: Merge this into the earlier set_flags() call.

Done.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim.h (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.h:30: // The most recent screen size.
On 2013/09/12 14:20:15, Wez wrote:
> nit: Suggest "Used to re-send screen-size only when it changes."

Done.

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim_unittest.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim_unittest.cc:15:
TestVideoEncoder(encoder.get(), false);
On 2013/09/12 14:20:15, Wez wrote:
> Why not strict?

strict makes the test verify that the rect specified in VideoPacketFormat
matches expected. Only the old verbatim codec used these fields. They are not
used anymore, so it doesn't make sense to check them. 

Basically the test was written with the assumption that we send one rect per
packet instead of one frame per packet. That doesn't make sense with either of
the codecs now.

Removed these fields from protobufs, cleaned up the test and changed this back
to true.

[Wez, 2013-09-13 11:12:20]

lgtm

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
File remoting/codec/video_encoder_verbatim.cc (right):

https://codereview.chromium.org/23477059/diff/12001/remoting/codec/video_enco...
remoting/codec/video_encoder_verbatim.cc:52: const int row_size =
webrtc::DesktopFrame::kBytesPerPixel * rect.width();
On 2013/09/12 19:18:45, Sergey Ulanov wrote:
> On 2013/09/12 14:20:15, Wez wrote:
> > nit: row_bytes or out_stride?
> 
> there is no out_stride, because it's not a single image - instead it's a
> sequence of rows of updated rectangles. This is is size of a single row for in
> the current rectangle.

Strictly no, but while processing a given rect |out| has a particular stride, so
I think the name would be reasonable.

https://codereview.chromium.org/23477059/diff/29001/remoting/codec/video_deco...
File remoting/codec/video_decoder.h (right):

https://codereview.chromium.org/23477059/diff/29001/remoting/codec/video_deco...
remoting/codec/video_decoder.h:19: // TODO(ajwong): Beef up this documentation
once the API stablizes.
nit: This comment can probably go away. :)

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.proto
File remoting/proto/video.proto (right):

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.prot...
remoting/proto/video.proto:44: // Depricated. Must be set to 7 for backward
compatibility.
typo: Deprecated.

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.prot...
remoting/proto/video.proto:44: // Depricated. Must be set to 7 for backward
compatibility.
IIUC the default configures what will be returned if a caller asks for flags(),
but has_flags() will still be false?  AFAICT nothing ever checked has_flags()?

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.prot...
remoting/proto/video.proto:53: // libvpx.
nit: Suggest "List of rectangles updated by this frame." for now.  We might omit
the list in some future codec variant, but I don't think that possibility needs
documenting here.

[Sergey Ulanov, 2013-09-13 18:53:15]

https://codereview.chromium.org/23477059/diff/29001/remoting/codec/video_deco...
File remoting/codec/video_decoder.h (right):

https://codereview.chromium.org/23477059/diff/29001/remoting/codec/video_deco...
remoting/codec/video_decoder.h:19: // TODO(ajwong): Beef up this documentation
once the API stablizes.
On 2013/09/13 11:12:21, Wez wrote:
> nit: This comment can probably go away. :)

Done.

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.proto
File remoting/proto/video.proto (right):

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.prot...
remoting/proto/video.proto:44: // Depricated. Must be set to 7 for backward
compatibility.
On 2013/09/13 11:12:21, Wez wrote:
> typo: Deprecated.

Done.

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.prot...
remoting/proto/video.proto:44: // Depricated. Must be set to 7 for backward
compatibility.
On 2013/09/13 11:12:21, Wez wrote:
> IIUC the default configures what will be returned if a caller asks for
flags(),
> but has_flags() will still be false?  AFAICT nothing ever checked has_flags()?

That's good point. What's important is that we send this flag when the message
is serialized, and it won't be added if has_flags() is not set. Changed
ProtobufVideoWriter to set this flag for all messages. Also renamed it to
deprecated_flags.

https://codereview.chromium.org/23477059/diff/29001/remoting/proto/video.prot...
remoting/proto/video.proto:53: // libvpx.
On 2013/09/13 11:12:21, Wez wrote:
> nit: Suggest "List of rectangles updated by this frame." for now.  We might
omit
> the list in some future codec variant, but I don't think that possibility
needs
> documenting here.

Done.

[commit-bot: I haz the power, 2013-09-13 19:05:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/23477059/24001

[Sergey Ulanov, 2013-09-13 22:45:07]

Committed patchset #5 manually as r223152 (presubmit successful).