Delete IsZeroSize method.

Delete IsZeroSize method.

BUG=webrtc:5682
This cl is superseded by https://codereview.webrtc.org/2541863002/

[nisse-webrtc, 2016-08-26 08:16:45]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 08:16:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-26 09:13:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 09:13:56]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-08-29 13:12:52]

nisse@webrtc.org changed reviewers:
+ mflodman@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2016-08-29 13:12:53]

Please have a look. 

IsZeroSize means that a VideoFrame has no associated VideoFrameBuffer, which
should be possible only for a default-constructed VideoFrame (I'd like to delete
the default constructors too, but not at the moment).

I haven't been able to understand why the IsZeroSize case needs to be handled by
the various codec classes.

[stefan-webrtc, 2016-09-05 14:45:25]

https://codereview.webrtc.org/2286633002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc (left):

https://codereview.webrtc.org/2286633002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/h264/h264_encoder_impl.cc:280: if
(input_frame.IsZeroSize()) {
Should we DCHECK that we have a video_frame_buffer, or is that not needed?

https://codereview.webrtc.org/2286633002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm
(left):

https://codereview.webrtc.org/2286633002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.mm:259:
RTC_DCHECK(!frame.IsZeroSize());
Could you comment on why this isn't necessary?

https://codereview.webrtc.org/2286633002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (left):

https://codereview.webrtc.org/2286633002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:611: if (!comparison.reference.IsZeroSize())
{
I'm pretty sure this was needed when I added this code. Why isn't it now?

[nisse-webrtc, 2016-09-06 06:23:08]

On 2016/09/05 14:45:25, stefan-webrtc (holmer) wrote:

>
https://codereview.webrtc.org/2286633002/diff/1/webrtc/video/video_quality_te...
> webrtc/video/video_quality_test.cc:611: if
(!comparison.reference.IsZeroSize())
> {
> I'm pretty sure this was needed when I added this code. Why isn't it now?

My question is really: Where and why does the code ever pass a
default-constructed VideoFrame (with video_frame_buffer() returning a nullptr)
to anywhere? To be able to delete IsZeroSize (or equivalent checks on
video_frame_buffer()), all those cases need to be tracked down and fixed. I was
hoping you might know...

If it's a difficult question, then I could try to get rid of the VideoFrame
default constructor first, that should surface any issues.

[stefan-webrtc, 2016-09-06 06:56:53]

On 2016/09/06 06:23:08, nisse-webrtc wrote:
> On 2016/09/05 14:45:25, stefan-webrtc (holmer) wrote:
> 
> >
>
https://codereview.webrtc.org/2286633002/diff/1/webrtc/video/video_quality_te...
> > webrtc/video/video_quality_test.cc:611: if
> (!comparison.reference.IsZeroSize())
> > {
> > I'm pretty sure this was needed when I added this code. Why isn't it now?
> 
> My question is really: Where and why does the code ever pass a
> default-constructed VideoFrame (with video_frame_buffer() returning a nullptr)
> to anywhere? To be able to delete IsZeroSize (or equivalent checks on
> video_frame_buffer()), all those cases need to be tracked down and fixed. I
was
> hoping you might know...
> 
> If it's a difficult question, then I could try to get rid of the VideoFrame
> default constructor first, that should surface any issues.

Sounds good, I agree it makes little sense and I unfortunately don't have the
answer. Most likely you are right in all cases but the frame comparison thing.

[stefan-webrtc, 2016-09-06 06:58:12]

On 2016/09/06 06:56:53, stefan-webrtc (holmer) wrote:
> On 2016/09/06 06:23:08, nisse-webrtc wrote:
> > On 2016/09/05 14:45:25, stefan-webrtc (holmer) wrote:
> > 
> > >
> >
>
https://codereview.webrtc.org/2286633002/diff/1/webrtc/video/video_quality_te...
> > > webrtc/video/video_quality_test.cc:611: if
> > (!comparison.reference.IsZeroSize())
> > > {
> > > I'm pretty sure this was needed when I added this code. Why isn't it now?
> > 
> > My question is really: Where and why does the code ever pass a
> > default-constructed VideoFrame (with video_frame_buffer() returning a
nullptr)
> > to anywhere? To be able to delete IsZeroSize (or equivalent checks on
> > video_frame_buffer()), all those cases need to be tracked down and fixed. I
> was
> > hoping you might know...
> > 
> > If it's a difficult question, then I could try to get rid of the VideoFrame
> > default constructor first, that should surface any issues.
> 
> Sounds good, I agree it makes little sense and I unfortunately don't have the
> answer. Most likely you are right in all cases but the frame comparison thing.

I think I can lg this if you verify that the perf test which uses the frame
comparison doesn't get worse.

[nisse-webrtc, 2016-10-31 15:04:56]

On 2016/09/06 06:23:08, nisse-webrtc wrote:

> My question is really: Where and why does the code ever pass a
> default-constructed VideoFrame (with video_frame_buffer() returning a nullptr)
> to anywhere?

I've finally found one such place.

See
https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/video...,
the function

int32_t VCMGenericEncoder::RequestFrame(
    const std::vector<FrameType>& frame_types) {
  RTC_DCHECK_RUNS_SERIALIZED(&race_checker_);
  VideoFrame image;
  return encoder_->Encode(image, NULL, &frame_types);
}

So here we have a use of the default constructor, and this case can be detected
by Encode by using IsZeroSize. And of course, I'd like to delete both the
default constructor and IsZeroSize.

Maybe you can explain what it's used for?

If it really is needed, we ought to change the first argument of Encode from
VideoFrame& to VideoFrame*, to allow passing a NULL pointer. Or add a separate
method for this signalling.

To me it appears useless, greping the encoder sources for IsZeroSize, the
typical use is

  if (input_image.IsZeroSize()) {
    return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
  }

which would make the above RequestFrame method always a no-op. But I could well
be missing something.

[nisse-webrtc, 2016-10-31 15:15:42]

On 2016/10/31 15:04:56, nisse-webrtc wrote:

> To me it appears useless, greping the encoder sources for IsZeroSize, the
> typical use is
> 
>   if (input_image.IsZeroSize()) {
>     return WEBRTC_VIDEO_CODEC_ERR_PARAMETER;
>   }
> 
> which would make the above RequestFrame method always a no-op. But I could
well
> be missing something.

And RequestFrame is used in one place,
https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/video...

Who knows this logic? It was latest touched by Peter Boström doing some locking
improvements, otherwise unchanged since 2013, cl
https://webrtc-codereview.appspot.com/2200008 by andresp@.

[nisse-webrtc, 2016-11-30 16:11:23]

Description was changed from

==========
Delete IsZeroSize method.

BUG=webrtc:5682
==========

to

==========
Delete IsZeroSize method.

BUG=webrtc:5682

This cl is superseded by https://codereview.webrtc.org/2541863002/
==========