I wonder if we have to define something, gnu_source like when checking mmap64 on Linux ?

Seems to work fine for me on raspbian stretch with meson build fwiw.

 cc  -Isubprojects/gst-plugins-good/sys/v4l2/gstvideo4linux2@sha -Isubprojects/gst-plugins-good/sys/v4l2 -I../subprojects/gst-plugins-good/sys/v4l2 -Isubprojects/gst-plugins-good -I../subprojects/gst-plugins-good -I../subprojects/gst-plugins-good/gst-libs -Isubprojects/gstreamer/libs -I../subprojects/gstreamer/libs -Isubprojects/gstreamer -I../subprojects/gstreamer -Isubprojects/gst-plugins-base/gst-libs -I../subprojects/gst-plugins-base/gst-libs -Isubprojects/gstreamer/gst -Isubprojects/gst-plugins-base/gst-libs/gst/video -I/usr/include/glib-2.0 -I/usr/lib/arm-linux-gnueabihf/glib-2.0/include -I/usr/include/gudev-1.0 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O2 -g -fvisibility=hidden -fno-strict-aliasing -fPIC -pthread -DHAVE_CONFIG_H -MD -MQ 'subprojects/gst-plugins-good/sys/v4l2/gstvideo4linux2@sha/gstv4l2object.c.o' -MF 'subprojects/gst-plugins-good/sys/v4l2/gstvideo4linux2@sha/gstv4l2object.c.o.d' -o 'subprojects/gst-plugins-good/sys/v4l2/gstvideo4linux2@sha/gstv4l2object.c.o' -c ../subprojects/gst-plugins-good/sys/v4l2/gstv4l2object.c

Thanks for the input, Tim E., are you building with Meson or Autotools ?

Please provide the details about how you are bidding this so we can test and fix.

I'm using this script to install gstreamer and I get the same error: https://gist.github.com/sphaero/02717b0b35501ad94863

Which distribution is that being run on?

Raspbian Stretch Lite with RPD

Log: https://pastebin.com/J6PQhE9X

(In reply to Nicolas Dufresne (stormer) from comment #3)
> Thanks for the input, Tim E., are you building with Meson or Autotools ?

Autotools - the error was on Raspbian Stretch 2017-11-29

If it's not hard for you, can you say how should i build with Meson?

Created attachment 368930 [details] [review]
patch with function wrappers

I also had this problem when cross-compiling gst-plugins-good with Yocto for an i.MX6 machine.

I think assigning system calls like close(), read() etc. directly to function pointers is unstable. So, instead, I add here simple wrapper functions that (a) are guaranteed to have a definition that is compatible with the function pointers and (b) internally just call the actual function.

I did not add a wrapper for ioctl(), because at the moment, I am unsure how to forward the varags properly. There seems to be no safe platform neutral way of doing this. I looked at how libv4l2 does it, but this library is known for being bug ridden. And using syscall() directly, I don't know if this is okay.

Review of attachment 368930 [details] [review]:

Why do you wrap them all ? Only mmap is problematic, all others are POSIX compliant.

Review of attachment 368930 [details] [review]:

Also, marking as need work, since you are not checking the overflow.

*** Bug 794155 has been marked as a duplicate of this bug. ***

Can someone share his config.log please so this issue don't stay blocked forever. On Linux/glibc, mmap64 variant should be used, if not, we have a bug in the check. For the rest, the checks are supposed to check the size of off_t, why doesn't it work for you ?

Thanks for pointing me here.

I'm using autotools to build the modules for i.MX6. config.log reports that off_t has 4 bytes. mmap64 is not declared. I simply cannot pass a 64 bit offset to mmap.

I would suggest to go the other way around: make the signature of mmap in v4l2object platform dependent and wrap the call to libv4l2. As far as I can tell only the allocator uses mmap and actually uses a __u32 for the offset. I don't understand, why we should use libv4l2 as definition how to use syscalls in GStreamer.

(In reply to Michael Tretter from comment #16)
> Thanks for pointing me here.
> 
> I'm using autotools to build the modules for i.MX6. config.log reports that
> off_t has 4 bytes. mmap64 is not declared. I simply cannot pass a 64 bit
> offset to mmap.
> 
> I would suggest to go the other way around: make the signature of mmap in
> v4l2object platform dependent and wrap the call to libv4l2. As far as I can
> tell only the allocator uses mmap and actually uses a __u32 for the offset.
> I don't understand, why we should use libv4l2 as definition how to use
> syscalls in GStreamer.

It's not related to GStreamer, the libv4l2 headers was copy pasted to make the defines, that's all. We can flip it up using arch dependent in the dynamic table, but I still need a decent wrapper to be implemented. Also, a proper patch would pick the wrapper only when needed (no mmap64, off_t being 32bit). I have no idea what you guys are doing/using, so someone have to provide that patch. I've tried on various platform and arch, and could not reproduce.

Btw, I realized that this line is wrong:

> sys/v4l2/gstv4l2object.c:#if SIZEOF_OFF_T == 8 && !defined(mmap64)

mmap64 is not always a macro, hence should not be tested with defined(). I bet all of your have mmap64, but as this code make it look like you don't have it. the last series to fix this issue also screwed up, because the mmap64 define is not used later.

Created attachment 369415 [details] [review]
v4l2: Fix support for 32bit mmap

This should fix it, can anyone confirm ?

The patch fixes the problem, but there are further issues:

My glibc actually has mmap64 with off_t as 8 bytes, but was not used, because configure is missing a check check for largefile support. I attach a patch that adds the check. The size of off_t is now reported as 8 bytes.

I still don't think that it is a good idea to copy from libv4l2 and use the GLib types for the prototypes. The function pointer prototypes should exactly match the prototypes of the function that is actually called. Therefore, ALL the pointers in V4l2Object that might point to syscalls should use the prototypes of the syscall and the libv4l2 calls should be wrapped accordingly.

Created attachment 369428 [details] [review]
0001-v4l2object-add-check-for-largefile-support-to-fix-of.patch

(In reply to Michael Tretter from comment #21)
> The patch fixes the problem, but there are further issues:
> 
> My glibc actually has mmap64 with off_t as 8 bytes, but was not used,
> because configure is missing a check check for largefile support. I attach a
> patch that adds the check. The size of off_t is now reported as 8 bytes.
> 
> I still don't think that it is a good idea to copy from libv4l2 and use the
> GLib types for the prototypes. The function pointer prototypes should
> exactly match the prototypes of the function that is actually called.

Have you read my patch ? I've change the offset to off_t. The other types does not matter, glib or not, they will be the same.

> Therefore, ALL the pointers in V4l2Object that might point to syscalls
> should use the prototypes of the syscall and the libv4l2 calls should be
> wrapped accordingly.

Again, read my last patch properly.

Review of attachment 369428 [details] [review]:

The new patch uses system defines signature, and wrap libv4l2 if needed. The code in libv4l2 is already failing on 32bit off_t if it overflow.

(In reply to Nicolas Dufresne (stormer) from comment #23)
> (In reply to Michael Tretter from comment #21)
> > The patch fixes the problem, but there are further issues:
> > 
> > My glibc actually has mmap64 with off_t as 8 bytes, but was not used,
> > because configure is missing a check check for largefile support. I attach a
> > patch that adds the check. The size of off_t is now reported as 8 bytes.
> > 
> > I still don't think that it is a good idea to copy from libv4l2 and use the
> > GLib types for the prototypes. The function pointer prototypes should
> > exactly match the prototypes of the function that is actually called.
> 
> Have you read my patch ? I've change the offset to off_t. The other types
> does not matter, glib or not, they will be the same.
> 
> > Therefore, ALL the pointers in V4l2Object that might point to syscalls
> > should use the prototypes of the syscall and the libv4l2 calls should be
> > wrapped accordingly.
> 
> Again, read my last patch properly.

I read your patch and I absolutely agree that it is the right solution. I also understand that the GLib types are the same and do not matter. No objection to that. However, I think it would be cleaner (only cleaner, not more correct or whatever), to use exactly the same types as the signature of the syscall, if it is a function pointer to a syscall.

(In reply to Nicolas Dufresne (stormer) from comment #24)
> Review of attachment 369428 [details] [review] [review]:
> 
> The new patch uses system defines signature, and wrap libv4l2 if needed. The
> code in libv4l2 is already failing on 32bit off_t if it overflow.

Correct, but the detection of off_t is still wrong. This check is needed in addition to the libv4l2 wrapper to correctly use mmap64 on 32bit arch, if the glibc has largefile support. Without the patch, off_t on 32bit arch is always 32bit even though the glibc supports 64bit off_t. You want to have both patches: the check for largefile support in the glibc and correct handling if off_t is 32 bits.

Hmm, it's a bit of a mess, the patch is not just a check (as you comment suggest) it enables large file support out-large. That though make me realize why libv4l2 interface does not use off_t, because it's not a stable type on 32bit.

I see that gst-omx, gst-streaming-server, gstreamer and gst-plugins-base. So yes, let's enable this, but let's also include -bad, -ugly, -ffmpeg just in case.

Also, we need to enable this in meson too.

Review of attachment 369428 [details] [review]:

Need meson side of fix the comment, it's enabling largefile support when supported.

Created attachment 369456 [details] [review]
0001-configure.ac-enable-largefile-support-if-possible.patch

I fixed the comment and changed the scope of the patch.

The patch applies on -good, -bad, and -ugly, but not -ffmpeg.

I have absolutely no experience with meson, but it looks like meson enables largefile support by default since 0.39.1: https://github.com/mesonbuild/meson/commit/853634a48da025c59eef70161dba0d150833f60d

Ah great, no work on meson then. Thanks for the patches, and sorry for the little oops. This has bee miss-fixed 3 times already, let's hope we got it right this time.

Review of attachment 369456 [details] [review]:

I'll apply on each repo and then check what's up with gst-ffmpeg (and if needed)

Review of attachment 369415 [details] [review]:

I'll change the signature to follow POSIX def.

(In reply to Nicolas Dufresne (stormer) from comment #32)
> Review of attachment 369415 [details] [review] [review]:
> 
> I'll change the signature to follow POSIX def.

I've skipped that part due to ambiguity between linux/posix/v4l2

Review of attachment 369415 [details] [review]:

Commited as 7d4702e2fb052adf716bdf02fd086cfc13aae9c9

Review of attachment 369456 [details] [review]:

Commited as f1141a4ac66b09f791c208601534ec20abb9500d

Created attachment 370033 [details] [review]
Fix the compilation error cuased by commit 7d4702e

commit 7d4702e causes a compilation error on Ubuntu 17.10

  CC       libgstvideo4linux2_la-gstv4l2allocator.lo
In file included from gstv4l2allocator.c:30:0:
gstv4l2object.h:197:17: error: unknown type name ‘off_t’; did you mean ‘__off_t’?
       gint fd,  off_t offset);
                 ^~~~~

No, I didn't mean __off_t gcc, cause only you defines this.

Can you tell us which include is missing that defines this on Ubuntu? It's weird that it supports mmap in the first place. I wonder if this isn't a -dev package screw up.

Both <sys/types.h> and <sys/mman.h> define off_t in the same way on Ubuntu 17.10

#ifndef __off_t_defined
# ifndef __USE_FILE_OFFSET64
typedef __off_t off_t;
# else
typedef __off64_t off_t;
# endif
# define __off_t_defined
#endif

The preprocessor output shows off_t is not defined when off_t is used in the first place, I may attach the preprocessor output if you need it. 

The issue still exists even if I reinstalled libc6-dev.

The error in comment #36 disappears if I included <sys/types.h> or <sys/mman.h> before gstv4l2object.h in gstv4l2allocator.c, but another error is exposed when compiling gstv4l2colorbalance.c

  CC       libgstvideo4linux2_la-gstv4l2colorbalance.lo
In file included from gstv4l2colorbalance.h:30:0,
                 from gstv4l2colorbalance.c:29:
gstv4l2object.h:197:17: error: unknown type name ‘off_t’; did you mean ‘__off_t’?
       gint fd,  off_t offset);
                 ^~~~~
                 __off_t


So it would be better to include <sys/types.h> or <sys/mman.h> in gstv4l2object.h

Yes, do you want to provide a patch ?

Actually, there was already a bug report with patches for this.

https://bugzilla.gnome.org/show_bug.cgi?id=794533

I am sorry I didn't search the bugzilla for this issue before. I provided a patch in comment #36 but the patch in https://bugzilla.gnome.org/show_bug.cgi?id=794533 also works for me.

Review of attachment 370033 [details] [review]:

As this patch is a dup of another.