Linux Video Capture: small cleanup

Linux Video Capture: small cleanup

I took the liberty to bundle a bunch of cleanups:

- Some loops of the form while(...){... increment_variable} are passed to
for loops, this IMHO brings closer the loop-specific actions.

- Const added to many local variables.

- 1 Const auto and range based loop.

- GetListOfUsableFourCCs -> changed to return a std::list<int>
instead of filling up a pointer parameter; IMHO this reflects
better the method name.

- Renamed V4l2ColorToVideoCaptureColorFormat() ->
V4l2FourCcToChromiumPixelFormat() to reflect better what is doing.

- Removed superfluous comments and added refs to crbug.coms.

BUG=441836
Committed: https://crrev.com/7b4d541404e82b7b12a2e8255e3529e95dac26a0
Cr-Commit-Position: refs/heads/master@{#308698}

[mcasas, 2014-12-12 19:23:11]

mcasas@chromium.org changed reviewers:
+ perkj@chromium.org, tommi@chromium.org

[mcasas, 2014-12-12 19:23:12]

tommi@/perkj@ PTAL.

[mcasas, 2014-12-12 19:53:53]

Patchset #1 (id:1) has been deleted

[mcasas, 2014-12-12 19:53:58]

Patchset #1 (id:20001) has been deleted

[mcasas, 2014-12-12 20:12:58]

Patchset #1 (id:40001) has been deleted

[mcasas, 2014-12-12 22:05:45]

Patchset #1 (id:60001) has been deleted

[mcasas, 2014-12-12 22:05:51]

Patchset #1 (id:80001) has been deleted

[mcasas, 2014-12-12 22:11:35]

Patchset #1 (id:100001) has been deleted

[mcasas, 2014-12-15 16:04:46]

ping.

[perkj_chrome, 2014-12-16 10:34:43]

Sorry, I had totally missed this.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_factory_linux.cc (right):

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:95: // output
devices, see http://crbug.com/139356.
? Can you expand on this?

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:99:
!(cap.capabilities & V4L2_CAP_VIDEO_OUTPUT)) &&
the indentation was corret from the beginning.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:149:
++frame_interval.index) {
nit indentation.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:77: return
HANDLE_EINTR(ioctl(device_fd.get(), request, arg));
I think this is less readable than calling HANDLE_EINTR(ioctl(device_fd.get(),
request, arg)) inline. 

codesearch seems to agree with me mostly, but not 100%. 
The virtual method Ioctl you reference seems to be there since there are
different implementations that need to be called.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:27: static int
Ioctl(const base::ScopedFD& device_fd, int request, void* arg);
please remove  Ioctl from public interface.

[mcasas, 2014-12-16 17:54:02]

Patchset #2 (id:140001) has been deleted

[mcasas, 2014-12-16 17:57:34]

Patchset #2 (id:160001) has been deleted

[mcasas, 2014-12-16 18:08:17]

PTAL

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_factory_linux.cc (right):

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:95: // output
devices, see http://crbug.com/139356.
On 2014/12/16 10:34:43, perkj wrote:
> ? Can you expand on this?

Done.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:99:
!(cap.capabilities & V4L2_CAP_VIDEO_OUTPUT)) &&
On 2014/12/16 10:34:43, perkj wrote:
> the indentation was corret from the beginning.

Done.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:149:
++frame_interval.index) {
On 2014/12/16 10:34:43, perkj wrote:
> nit indentation.

Done.

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:77: return
HANDLE_EINTR(ioctl(device_fd.get(), request, arg));
On 2014/12/16 10:34:43, perkj wrote:
> I think this is less readable than calling HANDLE_EINTR(ioctl(device_fd.get(),
> request, arg)) inline. 
> 
> codesearch seems to agree with me mostly, but not 100%. 
> The virtual method Ioctl you reference seems to be there since there are
> different implementations that need to be called.

Reverted then.

Just FTR I'd say it was cleaner to refer to a local Ioctl()
method that encapsulated the HANDLE_EINTR part, and would
homogeneise the <= 0, < 0, == -1 etc comparisons all over,
all meaning "if ok" or "if not ok".

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/804473007/diff/120001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.h:27: static int
Ioctl(const base::ScopedFD& device_fd, int request, void* arg);
On 2014/12/16 10:34:43, perkj wrote:
> please remove  Ioctl from public interface. 

Done.

(FTR, it was there to be called from VCDFactoryLinux).

[mcasas, 2014-12-16 19:11:28]

Patchset #2 (id:180001) has been deleted

[perkj_chrome, 2014-12-16 20:28:29]

lgtm if the below indentations problems are fixed.

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_factory_linux.cc (right):

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:34:
++fmtdesc.index) {
++ goes underneath ;

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:132: == 0;
++frame_size.index) {
Can you indent this the same way as you do on line 147.

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:156:
frame_interval.type == V4L2_FRMIVAL_TYPE_STEPWISE) {
indentation

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:245: ++fmtdesc.index) {
indentation

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:284: < 0) {
was correct from the beginning.

[mcasas, 2014-12-16 23:02:16]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-12-16 23:03:18]

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

[mcasas, 2014-12-16 23:50:31]

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_factory_linux.cc (right):

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:34:
++fmtdesc.index) {
On 2014/12/16 20:28:28, perkj wrote:
> ++ goes underneath ;

Done.

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:132: == 0;
++frame_size.index) {
On 2014/12/16 20:28:28, perkj wrote:
> Can you indent this the same way as you do on line 147.

Done.

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_factory_linux.cc:156:
frame_interval.type == V4L2_FRMIVAL_TYPE_STEPWISE) {
On 2014/12/16 20:28:28, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
File media/video/capture/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:245: ++fmtdesc.index) {
On 2014/12/16 20:28:28, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/804473007/diff/200001/media/video/capture/lin...
media/video/capture/linux/video_capture_device_linux.cc:284: < 0) {
On 2014/12/16 20:28:29, perkj wrote:
> was correct from the beginning.

Done.

[commit-bot: I haz the power, 2014-12-17 00:03:12]

Committed patchset #3 (id:220001)

[commit-bot: I haz the power, 2014-12-17 00:04:26]

Patchset 3 (id:??) landed as
https://crrev.com/7b4d541404e82b7b12a2e8255e3529e95dac26a0
Cr-Commit-Position: refs/heads/master@{#308698}