Re: [FFmpeg-devel] [PATCH 10/14] avformat/matroskadec: move the elements semantic in a separate file

2020-04-08 Thread Andreas Rheinhardt 
Steve Lhomme:
> From: Steve Lhomme 
> 
> So the file can be generated from the Matroska Schema.
> 
> The EbmlSyntax structures are not shared between files.
> matroska_segments and matroska_cluster_enter also have their size predefined.
> 
> No functional changes.
> ---
>  libavformat/Makefile  |   2 +-
>  libavformat/matroskadec.c | 668 +-
>  libavformat/matroskasem.c | 384 ++
>  libavformat/matroskasem.h | 362 +
>  4 files changed, 748 insertions(+), 668 deletions(-)
>  create mode 100644 libavformat/matroskasem.c
>  create mode 100644 libavformat/matroskasem.h
> 

[...]

> diff --git a/libavformat/matroskasem.h b/libavformat/matroskasem.h
> new file mode 100644
> index 00..8171982abf
> --- /dev/null
> +++ b/libavformat/matroskasem.h
> @@ -0,0 +1,362 @@
> +/*
> + * Matroska file semantic definition
> + * Copyright (c) 2003-2020 The FFmpeg project
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg 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.
> + *
> + * FFmpeg 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +
> +/**
> + * @file
> + * Matroska file demuxer
> + * @author Ronald Bultje 
> + * @author with a little help from Moritz Bunkus 
> + * @author totally reworked by Aurelien Jacobs 
> + * @see specs available on the Matroska project page: 
> http://www.matroska.org/
> + */
> +
> +#ifndef AVFORMAT_MATROSKASEM_H
> +#define AVFORMAT_MATROSKASEM_H
> +
> +#include 
> +

[...]

> +// The following forward declarations need their size because
> +// a tentative definition with internal linkage must not be an
> +// incomplete type (6.7.2 in C90, 6.9.2 in C99).
> +// Removing the sizes breaks MSVC.
> +EbmlSyntax ebml_syntax[3], matroska_segment[9], 
> matroska_track_video_color[15], matroska_track_video[19],
> +  matroska_track[27], matroska_track_encoding[6], 
> matroska_track_encodings[2],
> +  matroska_track_combine_planes[2], 
> matroska_track_operation[2], matroska_tracks[2],
> +  matroska_attachments[2], matroska_chapter_entry[9], 
> matroska_chapter[6], matroska_chapters[2],
> +  matroska_index_entry[3], matroska_index[2], 
> matroska_tag[3], matroska_tags[2], matroska_seekhead[2],
> +  matroska_blockadditions[2], matroska_blockgroup[8], 
> matroska_cluster_parsing[8];

1. This here has confused me a lot. But first a quote from 6.9.2.2 of C99:

"A declaration of an identifier for an object that has file scope
without an initializer, and without a storage-class specifier or with
the storage-class specifier static, constitutes a tentative definition.
If a translation unit contains one or more tentative definitions for an
identifier, and the translation unit contains no external definition for
that identifier, then the behavior is exactly as if the translation unit
contains a file scope declaration of that identifier, with the composite
type as of the end of the translation unit, with an initializer equal to 0."

This applies to the above as you have not declared these EbmlSyntax
arrays as extern. So according to the standard there will be one
instance of each of these arrays in every translation unit that includes
matroskasem.h. And given that these have external linkage this means
that these objects have two definitions. So it should not work.

And yet it did (to my confusion), but it won't work any more with GCC
10. Here's why:
"A common mistake in C is omitting extern when declaring a global
variable in a header file. If the header is included by several files it
results in multiple definitions of the same variable. In previous GCC
versions this error is ignored. GCC 10 defaults to -fno-common, which
means a linker error will now be reported. To fix this, use extern in
header files when declaring global variables, and ensure each global is
defined in exactly one C file." ([1])


Besides that, there is more wrong with your approach here:
2. "If the declaration of an identifier for an object is a tentative
definition and has internal linkage, the declared type shall not be an
incomplete type." (6.9.2.3 from C99; that's what the warning that you
just copied refers to) This means that one can (and should) remove the
sizes from all the arrays declared with external linkage.
3. You did not abide by

Re: [FFmpeg-devel] [PATCH 10/14] avformat/matroskadec: move the elements semantic in a separate file

2020-03-28 Thread Steve Lhomme 

On 2020-03-25 3:09, Andreas Rheinhardt wrote:

Steve Lhomme:

From: Steve Lhomme 

So the file can be generated from the Matroska Schema.

The EbmlSyntax structures are not shared between files.
matroska_segments and matroska_cluster_enter also have their size predefined.

No functional changes.
---
  libavformat/Makefile  |   2 +-
  libavformat/matroskadec.c | 668 +-
  libavformat/matroskasem.c | 384 ++
  libavformat/matroskasem.h | 362 +
  4 files changed, 748 insertions(+), 668 deletions(-)
  create mode 100644 libavformat/matroskasem.c
  create mode 100644 libavformat/matroskasem.h


[...]


diff --git a/libavformat/matroskasem.h b/libavformat/matroskasem.h
new file mode 100644
index 00..8171982abf
--- /dev/null
+++ b/libavformat/matroskasem.h
@@ -0,0 +1,362 @@
+/*
+ * Matroska file semantic definition
+ * Copyright (c) 2003-2020 The FFmpeg project
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg 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.
+ *
+ * FFmpeg 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * Matroska file demuxer
+ * @author Ronald Bultje 
+ * @author with a little help from Moritz Bunkus 
+ * @author totally reworked by Aurelien Jacobs 
+ * @see specs available on the Matroska project page: http://www.matroska.org/
+ */
+
+#ifndef AVFORMAT_MATROSKASEM_H
+#define AVFORMAT_MATROSKASEM_H
+
+#include 
+
+#include "matroska.h"
+#include "avformat.h"
+#include "libavutil/frame.h"


Is it possible that you just included this for AVBufferRef (which is
defined in libavutil/buffer.h)?


Sure, I'll give it a try.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 10/14] avformat/matroskadec: move the elements semantic in a separate file

2020-03-24 Thread Andreas Rheinhardt 
Steve Lhomme:
> From: Steve Lhomme 
> 
> So the file can be generated from the Matroska Schema.
> 
> The EbmlSyntax structures are not shared between files.
> matroska_segments and matroska_cluster_enter also have their size predefined.
> 
> No functional changes.
> ---
>  libavformat/Makefile  |   2 +-
>  libavformat/matroskadec.c | 668 +-
>  libavformat/matroskasem.c | 384 ++
>  libavformat/matroskasem.h | 362 +
>  4 files changed, 748 insertions(+), 668 deletions(-)
>  create mode 100644 libavformat/matroskasem.c
>  create mode 100644 libavformat/matroskasem.h

[...]

> diff --git a/libavformat/matroskasem.h b/libavformat/matroskasem.h
> new file mode 100644
> index 00..8171982abf
> --- /dev/null
> +++ b/libavformat/matroskasem.h
> @@ -0,0 +1,362 @@
> +/*
> + * Matroska file semantic definition
> + * Copyright (c) 2003-2020 The FFmpeg project
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg 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.
> + *
> + * FFmpeg 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +
> +/**
> + * @file
> + * Matroska file demuxer
> + * @author Ronald Bultje 
> + * @author with a little help from Moritz Bunkus 
> + * @author totally reworked by Aurelien Jacobs 
> + * @see specs available on the Matroska project page: 
> http://www.matroska.org/
> + */
> +
> +#ifndef AVFORMAT_MATROSKASEM_H
> +#define AVFORMAT_MATROSKASEM_H
> +
> +#include 
> +
> +#include "matroska.h"
> +#include "avformat.h"
> +#include "libavutil/frame.h"

Is it possible that you just included this for AVBufferRef (which is
defined in libavutil/buffer.h)?

- Andreas


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 10/14] avformat/matroskadec: move the elements semantic in a separate file

2020-03-22 Thread Steve Lhomme 
From: Steve Lhomme 

So the file can be generated from the Matroska Schema.

The EbmlSyntax structures are not shared between files.
matroska_segments and matroska_cluster_enter also have their size predefined.

No functional changes.
---
 libavformat/Makefile  |   2 +-
 libavformat/matroskadec.c | 668 +-
 libavformat/matroskasem.c | 384 ++
 libavformat/matroskasem.h | 362 +
 4 files changed, 748 insertions(+), 668 deletions(-)
 create mode 100644 libavformat/matroskasem.c
 create mode 100644 libavformat/matroskasem.h

diff --git a/libavformat/Makefile b/libavformat/Makefile
index 8fd0d43721..09cd0d49ce 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -294,7 +294,7 @@ OBJS-$(CONFIG_LVF_DEMUXER)   += lvfdec.o
 OBJS-$(CONFIG_LXF_DEMUXER)   += lxfdec.o
 OBJS-$(CONFIG_M4V_DEMUXER)   += m4vdec.o rawdec.o
 OBJS-$(CONFIG_M4V_MUXER) += rawenc.o
-OBJS-$(CONFIG_MATROSKA_DEMUXER)  += matroskadec.o matroska.o  \
+OBJS-$(CONFIG_MATROSKA_DEMUXER)  += matroskadec.o matroskasem.o 
matroska.o \
 rmsipr.o flac_picture.o \
 oggparsevorbis.o vorbiscomment.o \
 flac_picture.o replaygain.o
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index b021a4cce0..8aa2cbda61 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -54,6 +54,7 @@
 #include "internal.h"
 #include "isom.h"
 #include "matroska.h"
+#include "matroskasem.h"
 #include "oggdec.h"
 /* For ff_codec_get_id(). */
 #include "riff.h"
@@ -80,673 +81,6 @@
  * to this many bytes of unknown data 
for the
  * SKIP_THRESHOLD check. */
 
-typedef enum {
-EBML_NONE,
-EBML_UINT,
-EBML_SINT,
-EBML_FLOAT,
-EBML_STR,
-EBML_UTF8,
-EBML_BIN,
-EBML_NEST,
-EBML_LEVEL1,
-EBML_STOP,
-EBML_TYPE_COUNT
-} EbmlType;
-
-typedef const struct EbmlSyntax {
-uint32_t id;
-EbmlType type;
-size_t list_elem_size;
-size_t data_offset;
-union {
-int64_t i;
-uint64_tu;
-double  f;
-const char *s;
-const struct EbmlSyntax *n;
-} def;
-} EbmlSyntax;
-
-typedef struct EbmlList {
-int nb_elem;
-unsigned int alloc_elem_size;
-void *elem;
-} EbmlList;
-
-typedef struct EbmlBin {
-int  size;
-AVBufferRef *buf;
-uint8_t *data;
-int64_t  pos;
-} EbmlBin;
-
-typedef struct Ebml {
-uint64_t version;
-uint64_t max_size;
-uint64_t id_length;
-char*doctype;
-uint64_t doctype_version;
-} Ebml;
-
-typedef struct MatroskaTrackCompression {
-uint64_t algo;
-EbmlBin  settings;
-} MatroskaTrackCompression;
-
-typedef struct MatroskaTrackEncryption {
-uint64_t algo;
-EbmlBin  key_id;
-} MatroskaTrackEncryption;
-
-typedef struct MatroskaTrackEncoding {
-uint64_t scope;
-uint64_t type;
-MatroskaTrackCompression compression;
-MatroskaTrackEncryption encryption;
-} MatroskaTrackEncoding;
-
-typedef struct MatroskaMasteringMeta {
-double r_x;
-double r_y;
-double g_x;
-double g_y;
-double b_x;
-double b_y;
-double white_x;
-double white_y;
-double max_luminance;
-double min_luminance;
-} MatroskaMasteringMeta;
-
-typedef struct MatroskaTrackVideoColor {
-uint64_t matrix_coefficients;
-uint64_t bits_per_channel;
-uint64_t chroma_sub_horz;
-uint64_t chroma_sub_vert;
-uint64_t cb_sub_horz;
-uint64_t cb_sub_vert;
-uint64_t chroma_siting_horz;
-uint64_t chroma_siting_vert;
-uint64_t range;
-uint64_t transfer_characteristics;
-uint64_t primaries;
-uint64_t max_cll;
-uint64_t max_fall;
-MatroskaMasteringMeta mastering_meta;
-} MatroskaTrackVideoColor;
-
-typedef struct MatroskaTrackVideoProjection {
-uint64_t type;
-EbmlBin private;
-double yaw;
-double pitch;
-double roll;
-} MatroskaTrackVideoProjection;
-
-typedef struct MatroskaTrackVideo {
-double   frame_rate;
-uint64_t display_width;
-uint64_t display_height;
-uint64_t pixel_width;
-uint64_t pixel_height;
-EbmlBin  color_space;
-uint64_t display_unit;
-uint64_t interlaced;
-uint64_t field_order;
-uint64_t stereo_mode;
-uint64_t alpha_mode;
-EbmlList color;
-MatroskaTrackVideoProjection projection;
-} MatroskaTrackVideo;
-
-typedef struct MatroskaTrackAudio {
-double   samplerate;
-double   out_samplerate;
-uint64_t bitdepth;
-uint64_t channels;
-
-/* real audio header (extracted from extradata) */
-int  coded_framesize;
-int  sub_packet_h;
-int  frame_size;
-int  sub_packet_size;
-int  sub_packet_cnt;
-int  pkt_cnt;
-uint64_t