Hi,
On 08/09/2012 02:14 PM, Hans Verkuil wrote:
On Thu August 9 2012 13:58:07 Hans de Goede wrote:
Hi Konke,
As Gregor already mentioned there is no need to define libv4l2rdssubdir in configure.ac ,
so please drop that.
Other then that I've some minor remarks (comments inline), with all those
fixed, this one is could to go. So hopefully the next version can be added
to git master!
On 08/07/2012 05:11 PM, Konke Radlow wrote:
---
Makefile.am | 3 +-
configure.ac | 7 +-
lib/include/libv4l2rds.h | 228 ++++++++++
lib/libv4l2rds/Makefile.am | 11 +
lib/libv4l2rds/libv4l2rds.c | 953 +++++++++++++++++++++++++++++++++++++++
lib/libv4l2rds/libv4l2rds.pc.in | 11 +
6 files changed, 1211 insertions(+), 2 deletions(-)
create mode 100644 lib/include/libv4l2rds.h
create mode 100644 lib/libv4l2rds/Makefile.am
create mode 100644 lib/libv4l2rds/libv4l2rds.c
create mode 100644 lib/libv4l2rds/libv4l2rds.pc.in
<snip>
diff --git a/lib/include/libv4l2rds.h b/lib/include/libv4l2rds.h
new file mode 100644
index 0000000..4aa8593
--- /dev/null
+++ b/lib/include/libv4l2rds.h
@@ -0,0 +1,228 @@
+/*
+ * Copyright 2012 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
+ * Author: Konke Radlow <koradlow@xxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; either version 2.1 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Suite 500, Boston, MA 02110-1335 USA
+ */
+
+#ifndef __LIBV4L2RDS
+#define __LIBV4L2RDS
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdbool.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <time.h>
+#include <sys/types.h>
+#include <sys/mman.h>
+#include <config.h>
You should never include config.h in a public header, also
are all the headers really needed for the prototypes in this header?
I don't think so! Please move all the unneeded ones to the libv4l2rds.c
file!
+
+#include <linux/videodev2.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif /* __cplusplus */
+
+#if HAVE_VISIBILITY
+#define LIBV4L_PUBLIC __attribute__ ((visibility("default")))
+#else
+#define LIBV4L_PUBLIC
+#endif
+
+/* used to define the current version (version field) of the v4l2_rds struct */
+#define V4L2_RDS_VERSION (1)
+
What is the purpose of this field? Once we've released a v4l-utils with this
library we are stuck to the API we've defined, having a version field & changing it,
won't stop us from breaking existing apps, so once we've an official release we
simply cannot make ABI breaking changes, which is why most of my review sofar
has concentrated on the API side :)
I suggest dropping this define and the version field from the struct.
I think it is useful, actually. The v4l2_rds struct is allocated by the v4l2_rds_create
so at least in theory it is possible to extend the struct in the future without breaking
existing apps, provided you have a version number to check.
I disagree, if it gets extended only, then existing apps will just work, if an apps gets
compiled against a newer version with the extension then it is safe to assume it will run
against that newer version. The only reason I can see a version define being useful is
to make a newer app compile with an older version of the librarry, but that only requires
a version define, not a version field in the struct.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
