Refactored pixel format resize operations in media/video/capture into a function called VideoCaptu…

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

BUG=

TEST=VideoCaptureDeviceTest.* in media_unittests pass.

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

[emircan, 2015-02-02 17:55:52]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2015-02-02 17:56:33]

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

[commit-bot: I haz the power, 2015-02-02 17:56:34]

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

[commit-bot: I haz the power, 2015-02-02 17:56:35]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[emircan, 2015-02-02 17:57:44]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2015-02-02 17:58:49]

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

[commit-bot: I haz the power, 2015-02-02 17:58:50]

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

[commit-bot: I haz the power, 2015-02-02 17:58:52]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[emircan, 2015-02-02 18:00:15]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-02-02 18:00:16]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2015-02-02 18:53:29]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org

[emircan, 2015-02-02 19:26:47]

PTAL

[mcasas, 2015-02-02 19:46:46]

Needs some unit testing too.

https://codereview.chromium.org/897483002/diff/20001/media/video/capture/vide...
File media/video/capture/video_capture_types.cc (right):

https://codereview.chromium.org/897483002/diff/20001/media/video/capture/vide...
media/video/capture/video_capture_types.cc:36: return
VideoCaptureFormat::GetSizeForVideoPixelFormat(frame_size,
No need for address resolution (VideoCaptureFormat::).

https://codereview.chromium.org/897483002/diff/20001/media/video/capture/vide...
media/video/capture/video_capture_types.cc:43: size_t result_frame_size =
frame_size.GetArea();
const

https://codereview.chromium.org/897483002/diff/20001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/897483002/diff/20001/media/video/capture/vide...
media/video/capture/video_capture_types.h:71: size_t
GetSizeForVideoPixelFormat(VideoPixelFormat inputFormat) const;
Member functions knwo the |pixel_format_|, why pass it again?

https://codereview.chromium.org/897483002/diff/20001/media/video/capture/vide...
media/video/capture/video_capture_types.h:74: 
For the helper function we have 3 options:
- a member function (l.71), where you don't need to specify
"ForvideoPixelFormat", 'cause we'd want encapsulation. So,
naming would be more like GetSize().

- a class-static function, with similar comments.

- a helper media:: function, that should specify packed/planar
format and size as input parameters, and with a name indication
of the input parameters.

Is there anyone better than the other in terms of compilation/
runtime efficiency? Having a member call the static one looks
like overkill to me.



Besides, in a nutshell pixel formats can be:
- packed: where YUV or RGB or whatever pixel descriptions come
somehow back to back, in "one plane"
- planar: where the pixel descriptive magnitudes come in a number
of arrays, contiguous in memory or not.

In your case you need to reflect that this calculation is a
shortcut for the necessary amount of bytes needed to hold the
data, without making assumptions of the memory layout. Our
cousin VideoFrame calls it AllocationSize() [1], it might
inspire you ;)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...

[emircan, 2015-02-03 20:03:31]

PTAL.

I chose to declare it as a member function as all the calls are made within
VideoCaptureFormat instances and the required parameters are within the class
scope. 

For testing, instead of creating a separate function, I added checks to the
existing Capture* tests in VideoCaptureDeviceTest fixture. To provide further
code coverage, I added debug checks in video_capture controller where different
formats are covered.

[mcasas, 2015-02-03 20:48:46]

trybots please.

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

https://codereview.chromium.org/897483002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:430:
DCHECK_EQ(frame_format.ImageAllocationSize(),
Instead of reimplementing ImageAllocationSize() for
DCHECKing purposes, I'd test the result of that
method versus |length| input parameter. |length| 
should never be smaller than what is needed to hold
for a given pixel format, but it might be larger
due to padding and alignments.

You could add around l.487:
DCHECK_GE(length, frame_format.ImageAllocationSize());

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/file...
File media/video/capture/file_video_capture_device.cc (right):

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/file...
media/video/capture/file_video_capture_device.cc:173: int
FileVideoCaptureDevice::CalculateFrameSize() {
I suspect this method can be made const 
(in the definition, where it isn't).

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/vide...
File media/video/capture/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_unittest.cc:272:
EXPECT_EQ((size_t)640*480*3/2, last_format().ImageAllocationSize());
static_cast

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_unittest.cc:299:
EXPECT_EQ((size_t)1280*720*3/2, last_format().ImageAllocationSize());
idem

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_device_unittest.cc:325:
EXPECT_EQ((size_t)640*480*3/2, last_format().ImageAllocationSize());
idem, and all those remaining if any

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/vide...
media/video/capture/video_capture_types.h:70: // Returns the required image size
in bytes for a VideoCaptureFormat instance.
Needs more explanation: it'd be the size if tightly packed and
no padding etc.

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/win/...
File media/video/capture/win/sink_input_pin_win.cc (right):

https://codereview.chromium.org/897483002/diff/40001/media/video/capture/win/...
media/video/capture/win/sink_input_pin_win.cc:55:
requested_format_.ImageAllocationSize();
Doesn't this fit in one line? Same for the other changed lines
below.

[emircan, 2015-02-04 02:30:13]

PTAL

I used static_cast as suggested. I changed the unit test to check length at the
end of the function rather than case checks.

[emircan, 2015-02-04 03:59:05]

PTAL

[mcasas, 2015-02-04 17:00:02]

Can I haz bots please? :)

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

https://codereview.chromium.org/897483002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:471: 
Add a comment as to why _GE.

https://codereview.chromium.org/897483002/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_types.cc (right):

https://codereview.chromium.org/897483002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.cc:55: "GetSizeForVideoPixelFormat(): "
<< pixel_format;
Function name is outdated. I'd suggest using __FUNCTION__,
but that'd be already be provided by NOTREACHED(). So
I'd rewrite to sth like:
NOTREACHED() << "Unknown pixel format";

https://codereview.chromium.org/897483002/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/897483002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.h:70: // Returns the minimum required
image size in bytes for a VideoCaptureFormat
s/.../required buffer size to hold an image of a given VideoCaptureFormat./

https://codereview.chromium.org/897483002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.h:71: // instance.Return value is the
exact space required to tightly pack the data
Blank after .

https://codereview.chromium.org/897483002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.h:73: size_t ImageAllocationSize()
const;
Suggest re-writing like:

Returns the required buffer size to hold an image of a given VideoCaptureFormat,
with no padding and tightly packed if 
applicable.

[emircan, 2015-02-04 19:57:02]

PTAL.

Thanks for pointing out the commenting and spacing error. I am learning how to
use auto-formatting tools. 

I used trybots on Patch#4, but didn't do it on Patch#5 as the incremental change
was trivial.

[mcasas, 2015-02-04 23:02:39]

LGTM modulo the comments below.

(you need + reviews though).

https://codereview.chromium.org/897483002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/897483002/diff/140001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:472: // Given
length can be greater than the required buffer size when paddings are
s/when paddings are applied or alignments are changed/ because of paddings
and/or alignments/

https://codereview.chromium.org/897483002/diff/140001/media/video/capture/vid...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/897483002/diff/140001/media/video/capture/vid...
media/video/capture/video_capture_types.h:72: size_t ImageAllocationSize()
const;
I'm not totally in love with this name, despite being
the same in VideoFrame [1]. Please make a bug saying
that someone sometime should think about a better naming.
Hint: make the TODO look like [2].


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...

[emircan, 2015-02-05 17:14:23]

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

[emircan, 2015-02-06 00:11:38]

emircan@chromium.org changed reviewers:
+ perkj@chromium.org

[emircan, 2015-02-06 00:11:39]

PTAL.

[perkj_chrome, 2015-02-06 08:27:30]

lgtm if the todo is removed.

https://codereview.chromium.org/897483002/diff/200001/media/video/capture/vid...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/897483002/diff/200001/media/video/capture/vid...
media/video/capture/video_capture_types.h:71: // VideoCaptureFormat, with no
padding and tightly packed if applicable.
please remove the todo and update the comment here now. 
Aren't all formats currently tightly packed?

[emircan, 2015-02-06 17:12:04]

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

[emircan, 2015-02-06 17:20:33]

On 2015/02/06 08:27:30, perkj wrote:
> lgtm if the todo is removed.

I removed the todo.

>
https://codereview.chromium.org/897483002/diff/200001/media/video/capture/vid...
> media/video/capture/video_capture_types.h:71: // VideoCaptureFormat, with no
> padding and tightly packed if applicable.
> please remove the todo and update the comment here now. 
> Aren't all formats currently tightly packed?

I removed "if applicable" as they are all tightly packed at this stage as you
suggested.

[emircan, 2015-02-06 17:33:10]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2015-02-06 17:34:20]

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

[commit-bot: I haz the power, 2015-02-06 18:47:11]

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

[commit-bot: I haz the power, 2015-02-06 18:47:12]

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

[emircan, 2015-02-06 22:40:35]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2015-02-06 22:41:01]

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

[commit-bot: I haz the power, 2015-02-07 00:25:09]

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

[commit-bot: I haz the power, 2015-02-07 00:25:10]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12...)
  win_gpu_triggered_tests on tryserver.chromium.gpu (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[emircan, 2015-02-07 02:19:03]

The CQ bit was checked by emircan@chromium.org

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

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

[commit-bot: I haz the power, 2015-02-07 03:34:45]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-07 03:35:30]

Patchset 13 (id:??) landed as
https://crrev.com/79288ba0b63021e8731e0401b8a4a9055d33fa10
Cr-Commit-Position: refs/heads/master@{#315175}

[phoglund_chromium, 2015-02-07 12:45:00]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/898803004/ by phoglund@chromium.org.

The reason for reverting is: This breaks the WebRTC real webcam tests on
Windows. See for instance
http://chromegw.corp.google.com/i/internal.chromium.webrtc/builders/Win7%20Te....
The error is 

[2292:2232:0206/211958:997267:FATAL:video_capture_controller.cc(474)] Check
failed: static_cast<size_t>(length) >= frame_format.ImageAllocationSize() (0 vs.
614400)

The Win bots are equipped with C920 webcams. I logged into the bot to make sure
the webcam was working right - it is as far I can see..