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

[Andreas Rheinhardt]

Steve Lhomme:
> From: Steve Lhomme <robux4 at ycbcr.xyz>
> 
> 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 0000000000..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 <rbultje at ronald.bitfreak.net>
> + * @author with a little help from Moritz Bunkus <moritz at bunkus.org>
> + * @author totally reworked by Aurelien Jacobs <aurel at gnuage.org>
> + * @see specs available on the Matroska project page: http://www.matroska.org/
> + */
> +
> +#ifndef AVFORMAT_MATROSKASEM_H
> +#define AVFORMAT_MATROSKASEM_H
> +
> +#include <inttypes.h>
> +

[...]

> +// 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 our naming conventions [2]. You'd have to use an
ff_ prefix for all those arrays with external linkage.
4. It is not necessary to export everything; syntax structures not
directly referenced by matroskadec.c should be internal to matroskasem.c.

> +
> +EbmlSyntax matroska_segments[1];
> +EbmlSyntax matroska_cluster_enter[1];

5. These sizes are wrong as they do not account for the sentinel.

> +
> +#endif /* AVFORMAT_MATROSKASEM_H */
> 

- Andreas

[1]: https://gcc.gnu.org/gcc-10/porting_to.html
[2]: https://ffmpeg.org/developer.html#Naming-conventions