Reland:Refactored pixel format resize operations in media/video/capture into a function called Vide…

Reland:Refactored pixel format resize operations in media/video/capture into a function called VideoCaptureFormat::GetSizeForVideoPixelFormat().

Relanding Issue https://codereview.chromium.org/897483002.

Refactored pixel format resize operations in  media/video/capture into a function called VideoCaptureFormat::GetSizeForVideoPixelFormat().

TEST=VideoCaptureDeviceTest.* in media_unittests pass.

Committed: https://crrev.com/79288ba0b63021e8731e0401b8a4a9055d33fa10
Cr-Commit-Position: refs/heads/master@{#315175}

Committed: https://crrev.com/73c490dfcc4acd88d10f252b3029a89cd8fa5767
Cr-Commit-Position: refs/heads/master@{#317373}

[emircan, 2015-02-10 02:18:48]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org, perkj@google.com

[emircan, 2015-02-10 02:18:49]

PTAL.

Earlier commit fails showed that, on Windows platforms with real webcams, the
input |length| parameter for
VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedData() can
actually be 0. This |length| parameter is later on passed to
libyuv::ConvertToI420() as |sample_size|, and it is defined as "sample_size is
measured in bytes and is the size of the frame". Although having sample_size as
0 doesn't make much sense, we still need to take it into account. Therefore, I
changed the DCHECK on WIN platform to skip cases where the input |length| is 0.

[mcasas, 2015-02-10 03:35:22]

Patchset #2 (id:20001) has been deleted

[mcasas, 2015-02-10 03:37:51]

mcasas@chromium.org changed reviewers:
+ perkj@chromium.org
- perkj@google.com

[mcasas, 2015-02-10 03:37:52]

lgtm, my parts didn't change, only perkj@'s

[perkj_chrome, 2015-02-10 08:01:00]

Please also cleanup the cl description. 
If this does notfix an issue also remove BUG=

https://codereview.chromium.org/907113002/diff/1/media/video/capture/win/sink...
File media/video/capture/win/sink_input_pin_win.cc (right):

https://codereview.chromium.org/907113002/diff/1/media/video/capture/win/sink...
media/video/capture/win/sink_input_pin_win.cc:152: const int length =
sample->GetActualDataLength();
this is where length it set. Prefer to check lenght != 0 before calling
FrameReceived.

https://codereview.chromium.org/907113002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/907113002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:478:
static_cast<size_t>(length) >= frame_format.ImageAllocationSize());
please fix the problem at its source instead. ie, don't forward a frame of
length 0. See where in my other comment.

[perkj_chrome, 2015-02-10 08:05:45]

On 2015/02/10 08:01:00, perkj wrote:
> Please also cleanup the cl description. 
> If this does notfix an issue also remove BUG=
> 
>
https://codereview.chromium.org/907113002/diff/1/media/video/capture/win/sink...
> File media/video/capture/win/sink_input_pin_win.cc (right):
> 
>
https://codereview.chromium.org/907113002/diff/1/media/video/capture/win/sink...
> media/video/capture/win/sink_input_pin_win.cc:152: const int length =
> sample->GetActualDataLength();
> this is where length it set. Prefer to check lenght != 0 before calling
> FrameReceived.
> 
>
https://codereview.chromium.org/907113002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/907113002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/media/video_capture_controller.cc:478:
> static_cast<size_t>(length) >= frame_format.ImageAllocationSize());
> please fix the problem at its source instead. ie, don't forward a frame of
> length 0. See where in my other comment.

BTW, Feel free to TBR me if mcasas is happy after you addressed my comments.

[emircan, 2015-02-10 17:15:34]

On 2015/02/10 08:01:00, perkj wrote:

>
https://codereview.chromium.org/907113002/diff/1/media/video/capture/win/sink...
> media/video/capture/win/sink_input_pin_win.cc:152: const int length =
> sample->GetActualDataLength();
> this is where length it set. Prefer to check lenght != 0 before calling
> FrameReceived.

>
https://codereview.chromium.org/907113002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/media/video_capture_controller.cc:478:
> static_cast<size_t>(length) >= frame_format.ImageAllocationSize());
> please fix the problem at its source instead. ie, don't forward a frame of
> length 0. See where in my other comment.

Earlier commit failed, because C920 webcams on WIN would actually set this
length to 0. Phoglund@ stated that webcam was working right as well.
[https://codereview.chromium.org/898803004/] So, if we were to add a DCHECK at
sample->GetActualDataLength(), it would fail there. However, we can do take the
same WIN conditional DCHECK there if you think it is appropriate.

> BTW, Feel free to TBR me if mcasas is happy after you addressed my comments.

Thanks for taking time.

[perkj_chrome, 2015-02-12 15:24:58]

On 2015/02/10 17:15:34, emircan wrote:
> On 2015/02/10 08:01:00, perkj wrote:
> 
> >
>
https://codereview.chromium.org/907113002/diff/1/media/video/capture/win/sink...
> > media/video/capture/win/sink_input_pin_win.cc:152: const int length =
> > sample->GetActualDataLength();
> > this is where length it set. Prefer to check lenght != 0 before calling
> > FrameReceived.
> 
> >
>
https://codereview.chromium.org/907113002/diff/40001/content/browser/renderer...
> > content/browser/renderer_host/media/video_capture_controller.cc:478:
> > static_cast<size_t>(length) >= frame_format.ImageAllocationSize());
> > please fix the problem at its source instead. ie, don't forward a frame of
> > length 0. See where in my other comment.
> 
> Earlier commit failed, because C920 webcams on WIN would actually set this
> length to 0. Phoglund@ stated that webcam was working right as well.
> [https://codereview.chromium.org/898803004/] So, if we were to add a DCHECK at
> sample->GetActualDataLength(), it would fail there. However, we can do take
the
> same WIN conditional DCHECK there if you think it is appropriate.
> 
> > BTW, Feel free to TBR me if mcasas is happy after you addressed my comments.
> 
> Thanks for taking time.

Sorry if I missed this yesterday.
I meant that if the length is zero in sink_input_win, we should simply discard
the frame all together so it never reaches video_capture_controller.
That way you can have a DCHECK for all platforms in video_capture_controller.

[emircan, 2015-02-14 01:08:13]

New patchsets have been uploaded after l-g-t-m from mcasas@chromium.org

[emircan, 2015-02-18 23:01:19]

Patchset #3 (id:60001) has been deleted

[emircan, 2015-02-18 23:53:41]

Patchset #3 (id:80001) has been deleted

[emircan, 2015-02-18 23:55:35]

Patchset #3 (id:90008) has been deleted

[emircan, 2015-02-19 00:16:47]

On 2015/02/12 15:24:58, perkj wrote:
> I meant that if the length is zero in sink_input_win, we should simply discard
> the frame all together so it never reaches video_capture_controller.
> That way you can have a DCHECK for all platforms in video_capture_controller.

I added it to the earlier call SinkInputPin::Receive() as a DCHECK. Do you think
it should be a CHECK instead?

[perkj_chrome, 2015-02-19 07:05:05]

On 2015/02/19 00:16:47, emircan wrote:
> On 2015/02/12 15:24:58, perkj wrote:
> > I meant that if the length is zero in sink_input_win, we should simply
discard
> > the frame all together so it never reaches video_capture_controller.
> > That way you can have a DCHECK for all platforms in
video_capture_controller.
> 
> I added it to the earlier call SinkInputPin::Receive() as a DCHECK. Do you
think
> it should be a CHECK instead?

lgtm if you consider -
didn't you say that 0 might be a valid length on win from some cameras. If so, I
think we should just discard those frames in sink_input_pin instead of DCHECK?  
I don't think it should be a check.

[emircan, 2015-02-20 01:16:23]

New patchsets have been uploaded after l-g-t-m from perkj@chromium.org

[emircan, 2015-02-20 01:21:09]

On 2015/02/19 07:05:05, perkj wrote: 
> lgtm if you consider -
> didn't you say that 0 might be a valid length on win from some cameras. If so,
I
> think we should just discard those frames in sink_input_pin instead of DCHECK?
 
> I don't think it should be a check.

I see. I changed it to an early exit with DLOG.

[perkj_chrome, 2015-02-20 08:48:08]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2015-02-20 08:48:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907113002/130001

[commit-bot: I haz the power, 2015-02-20 12:39:55]

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

[commit-bot: I haz the power, 2015-02-20 12:39:56]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emircan, 2015-02-20 18:40:03]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2015-02-20 18:40:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907113002/150001

[commit-bot: I haz the power, 2015-02-20 19:34:43]

Committed patchset #5 (id:150001)

[commit-bot: I haz the power, 2015-02-20 19:35:26]

Patchset 5 (id:??) landed as
https://crrev.com/73c490dfcc4acd88d10f252b3029a89cd8fa5767
Cr-Commit-Position: refs/heads/master@{#317373}