[FFmpeg-devel] [PATCH]lavd/v4l2: Use ioctl(..., "int request" ) on Android

[Carl Eugen Hoyos]

Hi!

Attached patch fixes building with new Android toolchain, used to be a warning.

Please comment, Carl Eugen

[Carl Eugen Hoyos]

Post by Carl Eugen Hoyos
Hi!
Attached patch fixes building with new Android toolchain, used to be a warning.
Please comment, Carl Eugen
From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001
Date: Thu, 6 Dec 2018 23:34:54 +0100
Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for
ioctl()
on Android.
Fixes build with new Android toolchain.
---
libavdevice/v4l2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index 10a0ff0..aa7c052 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -95,7 +95,11 @@ struct video_data {
int (*open_f)(const char *file, int oflag, ...);
int (*close_f)(int fd);
int (*dup_f)(int fd);
+#ifdef __ANDROID__
+ int (*ioctl_f)(int fd, int request, ...);
+#else
int (*ioctl_f)(int fd, unsigned long int request, ...);
+#endif
ssize_t (*read_f)(int fd, void *buffer, size_t n);
void *(*mmap_f)(void *start, size_t length, int prot, int flags, int
fd, int64_t offset);
int (*munmap_f)(void *_start, size_t length);
--
1.7.10.4

LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for
the first case? Looking at possible V4L2-hosting libcs, only glibc has the
nonstandard* "unsigned long" rather than "int" as the request argument, so I
expect we're going to hit this in more cases (e.g. musl) if compilers are
now complaining about it.

Is videodev2.h a Linux header as opposed to a c-library header or am I
completely misunderstanding?

Carl Eugen

[Mark Thompson]

Post by Carl Eugen Hoyos

Post by Carl Eugen Hoyos
Hi!
Attached patch fixes building with new Android toolchain, used to be a warning.
Please comment, Carl Eugen
From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001
Date: Thu, 6 Dec 2018 23:34:54 +0100
Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for
ioctl()
on Android.
Fixes build with new Android toolchain.
---
libavdevice/v4l2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index 10a0ff0..aa7c052 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -95,7 +95,11 @@ struct video_data {
int (*open_f)(const char *file, int oflag, ...);
int (*close_f)(int fd);
int (*dup_f)(int fd);
+#ifdef __ANDROID__
+ int (*ioctl_f)(int fd, int request, ...);
+#else
int (*ioctl_f)(int fd, unsigned long int request, ...);
+#endif
ssize_t (*read_f)(int fd, void *buffer, size_t n);
void *(*mmap_f)(void *start, size_t length, int prot, int flags, int
fd, int64_t offset);
int (*munmap_f)(void *_start, size_t length);
--
1.7.10.4

LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for
the first case? Looking at possible V4L2-hosting libcs, only glibc has the
nonstandard* "unsigned long" rather than "int" as the request argument, so I
expect we're going to hit this in more cases (e.g. musl) if compilers are
now complaining about it.

Is videodev2.h a Linux header as opposed to a c-library header or am I
completely misunderstanding?

I'm not quite sure what you're asking, so I'll try to explain all of that more carefully:

videodev2.h is a Linux header, but it only defines the types and values to call ioctl() with so is mostly orthogonal to this discussion. ioctl() itself is a C library call, but since V4L2 is almost always used with glibc the prototype here has ended up matching that rather than the standard POSIX one.

Android follows the POSIX prototype, so your patch is correct. However, so do other standard C libraries such as Musl, and therefore I expect we will hit the same problem with other C libraries in the not-too-distant future. My suggestion, then, is to guard the change with "#ifdef __GLIBC__ <glibc-prototype> #else <posix-prototype> #endif", rather than fixing it only for Android as you have here.

Thanks,

- Mark

[Carl Eugen Hoyos]

Post by Mark Thompson

Post by Carl Eugen Hoyos

Post by Carl Eugen Hoyos
Hi!
Attached patch fixes building with new Android toolchain, used to be a warning.
Please comment, Carl Eugen
From d366c948af086520bfb2a4048e76f8d117690776 Mon Sep 17 00:00:00 2001
Date: Thu, 6 Dec 2018 23:34:54 +0100
Subject: [PATCH] lavd/v4l2: Use "int request" as second parameter for
ioctl()
on Android.
Fixes build with new Android toolchain.
---
libavdevice/v4l2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index 10a0ff0..aa7c052 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -95,7 +95,11 @@ struct video_data {
int (*open_f)(const char *file, int oflag, ...);
int (*close_f)(int fd);
int (*dup_f)(int fd);
+#ifdef __ANDROID__
+ int (*ioctl_f)(int fd, int request, ...);
+#else
int (*ioctl_f)(int fd, unsigned long int request, ...);
+#endif
ssize_t (*read_f)(int fd, void *buffer, size_t n);
void *(*mmap_f)(void *start, size_t length, int prot, int flags, int
fd, int64_t offset);
int (*munmap_f)(void *_start, size_t length);
--
1.7.10.4

LGTM on its own, but should this perhaps be "#ifndef (glibc something)" for
the first case? Looking at possible V4L2-hosting libcs, only glibc has the
nonstandard* "unsigned long" rather than "int" as the request argument, so I
expect we're going to hit this in more cases (e.g. musl) if compilers are
now complaining about it.

Is videodev2.h a Linux header as opposed to a c-library header or am I
completely misunderstanding?

videodev2.h is a Linux header, but it only defines the types and values to
call ioctl() with so is mostly orthogonal to this discussion.

I had not thought about this, you are of course right.

Post by Mark Thompson
ioctl() itself is a C library call, but since V4L2 is almost always used with glibc
the prototype here has ended up matching that rather than the standard POSIX
one.
Android follows the POSIX prototype, so your patch is correct. However, so
do other standard C libraries such as Musl, and therefore I expect we will
hit the same problem with other C libraries in the not-too-distant future.
My suggestion, then, is to guard the change with "#ifdef __GLIBC__
<glibc-prototype> #else <posix-prototype> #endif", rather than fixing it
only for Android as you have here.

Applied with your suggestion, thank you, Carl Eugen