Do not use libv4l if no plugin is registered.

Do not use libv4l if no plugin is registered.

v4l2_fd_open now returns -1 if no plugin is registered.
Do not use v4l2_ioctl and v4l2_close when there is no
plugin. This can reduce the overhead.

BUG=chrome-os-partner:38761
TEST=Play videos on CrOS veyron_jerry.

Committed: https://crrev.com/cd87e70f73b742263693831d42f4fed92d9b85c7
Cr-Commit-Position: refs/heads/master@{#326010}

[wuchengli, 2015-04-21 03:26:09]

Patchset #1 (id:1) has been deleted

[wuchengli, 2015-04-21 03:28:15]

wuchengli@chromium.org changed reviewers:
+ henryhsu@chromium.org, posciak@chromium.org

[wuchengli, 2015-04-21 03:28:16]

PTAL.

[Pawel Osciak, 2015-04-21 05:02:42]

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:66: close(device_fd_);
Perhaps this would be a good time to start using base::ScopedFd for both fds?

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:168: if
(HANDLE_EINTR(v4l2_fd_open(device_fd_, V4L2_DISABLE_CONVERSION)) >= 0) {
I'd prefer != -1.

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:169: DVLOG(2) << "Found a
libv4l2 plugin for " << device_path;
v4l2_fd_open succeeding is not exactly equivalent of finding a plugin. I know
right now it is, but I'd prefer not calling this out explicitly. Could we
instead say "using libvl42 for " or something like that?

[wuchengli, 2015-04-21 06:11:07]

Patchset #2 (id:40001) has been deleted

[wuchengli, 2015-04-21 06:12:10]

All done. PTAL.

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:66: close(device_fd_);
On 2015/04/21 05:02:42, Pawel Osciak wrote:
> Perhaps this would be a good time to start using base::ScopedFd for both fds?
Good idea. Done.

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:168: if
(HANDLE_EINTR(v4l2_fd_open(device_fd_, V4L2_DISABLE_CONVERSION)) >= 0) {
On 2015/04/21 05:02:42, Pawel Osciak wrote:
> I'd prefer != -1.
Done. I was confused when I discussed with you. != -1 is correct.

https://codereview.chromium.org/1093173002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:169: DVLOG(2) << "Found a
libv4l2 plugin for " << device_path;
On 2015/04/21 05:02:42, Pawel Osciak wrote:
> v4l2_fd_open succeeding is not exactly equivalent of finding a plugin. I know
> right now it is, but I'd prefer not calling this out explicitly. Could we
> instead say "using libvl42 for " or something like that?

Done.

[Pawel Osciak, 2015-04-21 09:19:47]

lgtm % nits

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:51: if (use_libv4l2_ &&
device_fd_ != -1)
Technically use_libv4l2_ is only true if device_fd_ != -1, but maybe better to
be more explicit and keep the check, since it's not obvious.

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.h (right):

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.h:57: bool use_libv4l2_;
Ah I realized, this should be under #ifdef USE_LIBV4L2 as well I think?

[wuchengli, 2015-04-21 09:25:24]

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:51: if (use_libv4l2_ &&
device_fd_ != -1)
On 2015/04/21 09:19:46, Pawel Osciak wrote:
> Technically use_libv4l2_ is only true if device_fd_ != -1, but maybe better to
> be more explicit and keep the check, since it's not obvious.
Right. My thought was the same -- keeping the check because it was not obvious.

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.h (right):

https://codereview.chromium.org/1093173002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.h:57: bool use_libv4l2_;
On 2015/04/21 09:19:46, Pawel Osciak wrote:
> Ah I realized, this should be under #ifdef USE_LIBV4L2 as well I think?
v4l2_close and v4l2_ioctl won't compile if USE_LIBV4L2 isn't defined. This one
is fine. So we don't need to add more #ifdef here and the constructor
initialization list. Also technically |use_libv4l2_| == false is correct when
USE_LIBV4L2 is not defined.

[wuchengli, 2015-04-21 09:38:32]

I upload a small new patchset to use ScopedGeneric::is_valid(). You can take a
look if want. If not, I'll commit this later today.

[Pawel Osciak, 2015-04-21 10:30:27]

lgtm

[wuchengli, 2015-04-21 11:41:02]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2015-04-21 11:41:14]

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

[commit-bot: I haz the power, 2015-04-21 11:44:16]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-04-21 11:45:08]

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