Add support for multiple V4L2 video devices of the same type.

Add support for multiple V4L2 video devices of the same type.

Platforms may expose multiple video decoders/encoders/etc. via separate
V4L2 video nodes. Each of them may support different codecs/formats.
Instead of handling only single /dev/video-dec, /dev/video-enc etc. device
nodes, enumerate all /dev/video-dec[0-9], /dev/video-enc[0-9] device nodes
present and choose one based on requested format/type.

BUG=chrome-os-partner:55497
TEST=vdatest,veatest,crosvideo.appspot

Committed: https://crrev.com/8eb65c7347b12340aacbee7dafe9783588a359ab
Cr-Commit-Position: refs/heads/master@{#424655}

[Pawel Osciak, 2016-10-06 01:14:33]

posciak@chromium.org changed reviewers:
+ rickyz@chromium.org

[Pawel Osciak, 2016-10-06 01:14:34]

rickyz@chromium.org: PTAL content/common/sandbox_linux/bpf_gpu_policy_linux.cc

Thank you!

[rickyz (no longer on Chrome), 2016-10-06 01:32:56]

content/common/sandbox_linux/bpf_gpu_policy_linux.cc lgtm

[Pawel Osciak, 2016-10-06 07:52:19]

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

[Pawel Osciak, 2016-10-06 07:52:19]

PTAL

[Pawel Osciak, 2016-10-06 07:52:28]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 07:52:42]

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

[commit-bot: I haz the power, 2016-10-06 09:01:05]

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

[commit-bot: I haz the power, 2016-10-06 09:01:06]

Dry run: This issue passed the CQ dry run.

[kcwu, 2016-10-06 10:40:00]

https://chromiumcodereview.appspot.com/2398883002/diff/20001/content/common/s...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/content/common/s...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:161: static const
base::FilePath::CharType kVideoDecPattern[] = "video-dec[0-9]";
How about video-enc[0-9], image-proc[0-9], jpeg-dec[0-9]?

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
File media/gpu/generic_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:146: DVLOG(1) << "No devices supporting " <<
std::hex << v4l2_pixfmt;
prepend "0x"

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:299: std::vector<uint32_t>
supported_pixelformats;
DCHECK_EQ(type_, Type::kImageProcessor);

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:313: }
CloseDevice();

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:334: }
CloseDevice();

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:353: }
CloseDevice();

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:364: CloseDevice();
I know you auto close before open. But I'm wondering is this a good idea to
encourage to write code without close?
If so, it may leave fd opened unnecessary long time like GetSupported*Profiles
above.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:435: for (int i = 0; i < 10; ++i) {
Is it possible that number > 9? Add comments if you intended to ignore them.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:448: continue;
CloseDevice?

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:464: DCHECK_NE(devices_by_type_.count(type),
0u);
Is this always true?

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
File media/gpu/generic_v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.h:26: bool Initialize() override;
Why put line 26 and 28 before line 30?

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
File media/gpu/v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.cc:145: NOTREACHED() << "Unhandled pixelformat " <<
std::hex << pix_fmt;
add "0x" string

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.cc:176: DVLOG(1) << "Unrecognized format " << std::hex <<
format;
"0x"

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.cc:361: V4L2Device::EnumerateSupportedEncodeProfiles() {
 DCHECK_EQ(type_, Type::kEncode);

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
File media/gpu/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.h:142: virtual std::vector<uint32_t>
GetSupportedImageProcessorPixelformats(
These new added apis all have side-effects.
 1. The Open'ed fd may be closed.
 2. After calls finished, it may leave unrelated fd open.

2 is easier to fix. For 1, I guess there is no simple elegant solution. Please
add comments about these side-effects if you to keep them. Maybe require users
don't call these functions after Open'ed something?

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_j...
File media/gpu/v4l2_jpeg_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_j...
media/gpu/v4l2_jpeg_decode_accelerator.cc:456: if (input_buffer_map_.empty())
Is this only for optimization? If so, how about separate CL?

[Pawel Osciak, 2016-10-07 08:03:34]

Patchset #3 (id:40001) has been deleted

[Pawel Osciak, 2016-10-07 08:16:31]

Patchset #3 (id:60001) has been deleted

[Pawel Osciak, 2016-10-07 08:21:04]

Patchset #3 (id:80001) has been deleted

[Pawel Osciak, 2016-10-07 08:24:44]

Patchset #3 (id:100001) has been deleted

[Pawel Osciak, 2016-10-07 08:30:24]

https://chromiumcodereview.appspot.com/2398883002/diff/20001/content/common/s...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/content/common/s...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:161: static const
base::FilePath::CharType kVideoDecPattern[] = "video-dec[0-9]";
On 2016/10/06 10:39:59, kcwu wrote:
> How about video-enc[0-9], image-proc[0-9], jpeg-dec[0-9]?

Since we are not using these yet, I thought it would be better not to open the
sandbox for them for now.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
File media/gpu/generic_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:146: DVLOG(1) << "No devices supporting " <<
std::hex << v4l2_pixfmt;
On 2016/10/06 10:39:59, kcwu wrote:
> prepend "0x" 

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:299: std::vector<uint32_t>
supported_pixelformats;
On 2016/10/06 10:39:59, kcwu wrote:
> DCHECK_EQ(type_, Type::kImageProcessor);

Hm, thinking more about it, I think I can reorganize type_ in general.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:313: }
On 2016/10/06 10:39:59, kcwu wrote:
> CloseDevice();

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:334: }
On 2016/10/06 10:39:59, kcwu wrote:
> CloseDevice();

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:353: }
On 2016/10/06 10:39:59, kcwu wrote:
> CloseDevice();

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:364: CloseDevice();
On 2016/10/06 10:39:59, kcwu wrote:
> I know you auto close before open. But I'm wondering is this a good idea to
> encourage to write code without close?
> If so, it may leave fd opened unnecessary long time like GetSupported*Profiles
> above.

Yes, this is not a good style. Fixing.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:435: for (int i = 0; i < 10; ++i) {
On 2016/10/06 10:39:59, kcwu wrote:
> Is it possible that number > 9? Add comments if you intended to ignore them.

Unfortunately we can't use an actual FileEnumerator, as we are already sandboxed
here and can't query directory contents. I'll add a comment.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:448: continue;
On 2016/10/06 10:39:59, kcwu wrote:
> CloseDevice?

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.cc:464: DCHECK_NE(devices_by_type_.count(type),
0u);
On 2016/10/06 10:39:59, kcwu wrote:
> Is this always true?

Yes. EnumerateDevicesForType() will always insert into devices_by_type_[type],
but the value of the element (typedef Devices) may be an empty vector if no
devices of type are present. This is intentional to indicate there are no
Devices of given Type and we check for this in callers of this method.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
File media/gpu/generic_v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/generi...
media/gpu/generic_v4l2_device.h:26: bool Initialize() override;
On 2016/10/06 10:39:59, kcwu wrote:
> Why put line 26 and 28 before line 30?

Ah right, good point.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
File media/gpu/v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.cc:145: NOTREACHED() << "Unhandled pixelformat " <<
std::hex << pix_fmt;
On 2016/10/06 10:40:00, kcwu wrote:
> add "0x" string

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.cc:176: DVLOG(1) << "Unrecognized format " << std::hex <<
format;
On 2016/10/06 10:39:59, kcwu wrote:
> "0x"

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.cc:361: V4L2Device::EnumerateSupportedEncodeProfiles() {
On 2016/10/06 10:39:59, kcwu wrote:
>  DCHECK_EQ(type_, Type::kEncode);

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
File media/gpu/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_d...
media/gpu/v4l2_device.h:142: virtual std::vector<uint32_t>
GetSupportedImageProcessorPixelformats(
On 2016/10/06 10:40:00, kcwu wrote:
> These new added apis all have side-effects.
>  1. The Open'ed fd may be closed.
>  2. After calls finished, it may leave unrelated fd open.
> 
> 2 is easier to fix. For 1, I guess there is no simple elegant solution. Please
> add comments about these side-effects if you to keep them. Maybe require users
> don't call these functions after Open'ed something?

Yes, I also wasn't happy about 1, it should be handled, but involves additional
reorganization and this CL is already pretty complicated. To not to make this CL
even larger, I'll add a comment and do this in a follow-up CL.

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_j...
File media/gpu/v4l2_jpeg_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/20001/media/gpu/v4l2_j...
media/gpu/v4l2_jpeg_decode_accelerator.cc:456: if (input_buffer_map_.empty())
On 2016/10/06 10:40:00, kcwu wrote:
> Is this only for optimization? If so, how about separate CL?

It's possible for this class to be created without actually opening the device.
In that case we may call an ioctl on an invalid fd, since this is called from
the destructor unconditionally.

If there are no buffers allocated, we don't need to call anything, so I feel
it's a good way to prevent that.

[Pawel Osciak, 2016-10-07 08:30:48]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 08:31:04]

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

[wuchengli1, 2016-10-07 08:40:28]

wuchengli@google.com changed reviewers:
+ wuchengli@google.com

[wuchengli1, 2016-10-07 08:40:29]

v4l2_video_decode_accelerator.cc LGTM

[commit-bot: I haz the power, 2016-10-07 09:42:59]

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

[commit-bot: I haz the power, 2016-10-07 09:43:00]

Dry run: This issue passed the CQ dry run.

[kcwu, 2016-10-07 11:19:05]

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/gener...
File media/gpu/generic_v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/gener...
media/gpu/generic_v4l2_device.h:27: bool Initialize() override;
This method is not expected call by users. I guess Initialize() could be
private.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
File media/gpu/tegra_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:59: RTLD_NOW | RTLD_GLOBAL | RTLD_NODELETE))
Why removed the DLOG ? It may silently fallback to generic v4l2 device and we
don't notice it.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:152: const char* device_path = nullptr;
DCHECK_EQ(device_fd_, -1);

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:250: return
EnumerateSupportedDecodeProfiles(num_formats, pixelformats);
don't forget to close, otherwise fd leak.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:258: return EnumerateSupportedEncodeProfiles();
don't forget to close, otherwise fd leak.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
File media/gpu/tegra_v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.h:28: bool Initialize() override;
This method is not expected call by users. I guess Initialize() could be
private.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:27: scoped_refptr<TegraV4L2Device> tegra_device(new
TegraV4L2Device());
Change it to scoped_refptr<V4L2Device> in order to make it possible to make
Initialize() private.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:32: scoped_refptr<GenericV4L2Device> generic_device(new
GenericV4L2Device());
Change it to scoped_refptr<V4L2Device> in order to make it possible to make
Initialize() private.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:61: DVLOG(1) << "Add more cases as needed";
How about DLOG(FATAL) ? anyway, up to you.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:145: DVLOG(1) << "Unhandled pixelformat " << std::hex
<< "0x" << pix_fmt;
"0x" could be merged with previous string literal. Up to you, if you feel this
is more readable.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:318: DVLOG(1) << "Found " << std::hex <<
fmtdesc.pixelformat << " "
0x

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:57: // Create and initialize an appropriate V4L2Device
instance for the current
s/Create/Creates/, s/initialize/initializes/

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:62: // Return true on success.
Add comment the dtor will close automatically.
Otherwise it is confusing why there is no Close() paired with Open().

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:171: virtual bool Initialize() = 0;
Please keep its comments for children classes.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_image_processor.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_image_processor.cc:141: << " fourcc: " << std::hex <<
input_format_fourcc_;
0x

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_image_processor.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_image_processor.h:58: // Return true if image processing is
supported on this platform.
s/Return/Returns/

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_jpeg_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_jpeg_decode_accelerator.cc:452: DCHECK(!input_streamon_);
I found this check is contradiction to line 459.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_jpeg_decode_accelerator.cc:482: DCHECK(!output_streamon_);
I found this check is contradiction to line 489.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_slice_video_decode_accelerator.cc:575: << " fourcc: " << std::hex
<< input_format_fourcc_;
0x

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_video_decode_accelerator.cc:258: << " fourcc: " << std::hex <<
input_format_fourcc_;
0x

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_video_decode_accelerator.cc:2150: image_processor_device_ =
nullptr;
DCHECK(image_processor_device_ == nullptr) at line 2121 instead.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_video_encode_accelerator.cc:166: <<
GetProfileName(output_profile) << ", fourcc=" << std::hex
0x

[Pawel Osciak, 2016-10-11 06:13:46]

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/gener...
File media/gpu/generic_v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/gener...
media/gpu/generic_v4l2_device.h:27: bool Initialize() override;
On 2016/10/07 11:19:04, kcwu wrote:
> This method is not expected call by users. I guess Initialize() could be
> private.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
File media/gpu/tegra_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:59: RTLD_NOW | RTLD_GLOBAL | RTLD_NODELETE))
On 2016/10/07 11:19:04, kcwu wrote:
> Why removed the DLOG ? It may silently fallback to generic v4l2 device and we
> don't notice it.

It is now the normal flow to try TegraV4L2Device first and then fall back to
GenericV4L2Device, so this would always print on non-Tegra devices.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:152: const char* device_path = nullptr;
On 2016/10/07 11:19:04, kcwu wrote:
> DCHECK_EQ(device_fd_, -1);

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:250: return
EnumerateSupportedDecodeProfiles(num_formats, pixelformats);
On 2016/10/07 11:19:04, kcwu wrote:
> don't forget to close, otherwise fd leak.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.cc:258: return EnumerateSupportedEncodeProfiles();
On 2016/10/07 11:19:04, kcwu wrote:
> don't forget to close, otherwise fd leak.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
File media/gpu/tegra_v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/tegra...
media/gpu/tegra_v4l2_device.h:28: bool Initialize() override;
On 2016/10/07 11:19:04, kcwu wrote:
> This method is not expected call by users. I guess Initialize() could be
> private.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:27: scoped_refptr<TegraV4L2Device> tegra_device(new
TegraV4L2Device());
On 2016/10/07 11:19:04, kcwu wrote:
> Change it to scoped_refptr<V4L2Device> in order to make it possible to make
> Initialize() private.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:32: scoped_refptr<GenericV4L2Device> generic_device(new
GenericV4L2Device());
On 2016/10/07 11:19:05, kcwu wrote:
> Change it to scoped_refptr<V4L2Device> in order to make it possible to make
> Initialize() private.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:61: DVLOG(1) << "Add more cases as needed";
On 2016/10/07 11:19:05, kcwu wrote:
> How about DLOG(FATAL) ? anyway, up to you.

Thinking more about this, we should probably log only on unrecognized V4L2 pixel
formats, because we may have a device with unsupported/unused by us format and
it's ok to skip it. On the other hand, if we go from Chrome format to V4L2
pixfmt, we are probably requesting to use it and need it, so this should
probably be fatal always. Changing with this in mind.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:145: DVLOG(1) << "Unhandled pixelformat " << std::hex
<< "0x" << pix_fmt;
On 2016/10/07 11:19:04, kcwu wrote:
> "0x" could be merged with previous string literal. Up to you, if you feel this
> is more readable.

Yes, I felt this was more readable.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.cc:318: DVLOG(1) << "Found " << std::hex <<
fmtdesc.pixelformat << " "
On 2016/10/07 11:19:04, kcwu wrote:
> 0x

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:57: // Create and initialize an appropriate V4L2Device
instance for the current
On 2016/10/07 11:19:05, kcwu wrote:
> s/Create/Creates/, s/initialize/initializes/

My personal preference when writing docs is usually the non-s version, I could
change if the whole class was in the s-style to match, but we already have a mix
of both in here, so if you don't feel strongly about this, I'd prefer this
version...

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:62: // Return true on success.
On 2016/10/07 11:19:05, kcwu wrote:
> Add comment the dtor will close automatically.
> Otherwise it is confusing why there is no Close() paired with Open().

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:171: virtual bool Initialize() = 0;
On 2016/10/07 11:19:05, kcwu wrote:
> Please keep its comments for children classes.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_image_processor.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_image_processor.cc:141: << " fourcc: " << std::hex <<
input_format_fourcc_;
On 2016/10/07 11:19:05, kcwu wrote:
> 0x

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_image_processor.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_image_processor.h:58: // Return true if image processing is
supported on this platform.
On 2016/10/07 11:19:05, kcwu wrote:
> s/Return/Returns/

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_jpeg_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_jpeg_decode_accelerator.cc:452: DCHECK(!input_streamon_);
On 2016/10/07 11:19:05, kcwu wrote:
> I found this check is contradiction to line 459.

My guess is that the intention here could have been that normally streamoff
should have been called, but this method is intended to clean up no matter what,
so in debug builds it will check to give a chance to fix the code, in Release
just try to do this anyway just in case.
But if there is an issue, we should rather try to fix it, than do this, but out
of scope of this CL though I'd say.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_jpeg_decode_accelerator.cc:482: DCHECK(!output_streamon_);
On 2016/10/07 11:19:05, kcwu wrote:
> I found this check is contradiction to line 489.

Please see above.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_slice_video_decode_accelerator.cc:575: << " fourcc: " << std::hex
<< input_format_fourcc_;
On 2016/10/07 11:19:05, kcwu wrote:
> 0x

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_video_decode_accelerator.cc:258: << " fourcc: " << std::hex <<
input_format_fourcc_;
On 2016/10/07 11:19:05, kcwu wrote:
> 0x

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_video_decode_accelerator.cc:2150: image_processor_device_ =
nullptr;
On 2016/10/07 11:19:05, kcwu wrote:
> DCHECK(image_processor_device_ == nullptr) at line 2121 instead.

Done.

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_video_encode_accelerator.cc:166: <<
GetProfileName(output_profile) << ", fourcc=" << std::hex
On 2016/10/07 11:19:05, kcwu wrote:
> 0x

Done.

[Pawel Osciak, 2016-10-11 06:13:59]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 06:14:20]

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

[kcwu, 2016-10-11 06:45:06]

lgtm

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
File media/gpu/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/120001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:57: // Create and initialize an appropriate V4L2Device
instance for the current
On 2016/10/11 06:13:46, Pawel Osciak wrote:
> On 2016/10/07 11:19:05, kcwu wrote:
> > s/Create/Creates/, s/initialize/initializes/
> 
> My personal preference when writing docs is usually the non-s version, I could
> change if the whole class was in the s-style to match, but we already have a
mix
> of both in here, so if you don't feel strongly about this, I'd prefer this
> version...

It's style guide's suggestion. I don't feel strongly about this.

https://chromiumcodereview.appspot.com/2398883002/diff/140001/media/gpu/v4l2_...
File media/gpu/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/2398883002/diff/140001/media/gpu/v4l2_...
media/gpu/v4l2_device.h:184: virtual bool Initialize() = 0;
I didn't know this could be private. Learnt one more thing :)

[commit-bot: I haz the power, 2016-10-11 07:31:15]

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

[commit-bot: I haz the power, 2016-10-11 07:31:16]

Dry run: This issue passed the CQ dry run.

[Pawel Osciak, 2016-10-12 02:29:59]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-10-12 02:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org,
wuchengli@google.com
Link to the patchset:
https://chromiumcodereview.appspot.com/2398883002/#ps140001 (title: "comments
addressed")

[commit-bot: I haz the power, 2016-10-12 02:30:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 02:34:25]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-10-12 02:37:47]

Description was changed from

==========
Add support for multiple V4L2 video devices of the same type.

Platforms may expose multiple video decoders/encoders/etc. via separate
V4L2 video nodes. Each of them may support different codecs/formats.
Instead of handling only single /dev/video-dec, /dev/video-enc etc. device
nodes, enumerate all /dev/video-dec[0-9], /dev/video-enc[0-9] device nodes
present and choose one based on requested format/type.

BUG=chrome-os-partner:55497
TEST=vdatest,veatest,crosvideo.appspot
==========

to

==========
Add support for multiple V4L2 video devices of the same type.

Platforms may expose multiple video decoders/encoders/etc. via separate
V4L2 video nodes. Each of them may support different codecs/formats.
Instead of handling only single /dev/video-dec, /dev/video-enc etc. device
nodes, enumerate all /dev/video-dec[0-9], /dev/video-enc[0-9] device nodes
present and choose one based on requested format/type.

BUG=chrome-os-partner:55497
TEST=vdatest,veatest,crosvideo.appspot

Committed: https://crrev.com/8eb65c7347b12340aacbee7dafe9783588a359ab
Cr-Commit-Position: refs/heads/master@{#424655}
==========

[commit-bot: I haz the power, 2016-10-12 02:37:48]

Patchset 4 (id:??) landed as
https://crrev.com/8eb65c7347b12340aacbee7dafe9783588a359ab
Cr-Commit-Position: refs/heads/master@{#424655}