[FFmpeg-devel] [PATCH 4/6] avformat/argo_asf: add version_major and version_minor options

[Andreas Rheinhardt]

Zane van Iperen:
> Signed-off-by: Zane van Iperen <zane at zanevaniperen.com>
> ---
>  libavformat/argo_asf.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/argo_asf.c b/libavformat/argo_asf.c
> index 1770192aad..9845cb955b 100644
> --- a/libavformat/argo_asf.c
> +++ b/libavformat/argo_asf.c
> @@ -23,6 +23,7 @@
>  #include "internal.h"
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/avassert.h"
> +#include "libavutil/opt.h"
>  
>  #define ASF_TAG                 MKTAG('A', 'S', 'F', '\0')
>  #define ASF_FILE_HEADER_SIZE    24
> @@ -64,6 +65,7 @@ enum {
>  };
>  
>  typedef struct ArgoASFContext {
> +    const AVClass      *class;
>      ArgoASFFileHeader   fhdr;
>      ArgoASFChunkHeader  ckhdr;
>      uint32_t            blocks_read;
> @@ -296,8 +298,7 @@ static int argo_asf_write_header(AVFormatContext *s)
>      ArgoASFContext          *ctx = s->priv_data;
>  
>      ctx->fhdr.magic              = ASF_TAG;
> -    ctx->fhdr.version_major      = 2;
> -    ctx->fhdr.version_minor      = 1;
> +    /* version_{major,minor} set by options. */
>      ctx->fhdr.num_chunks         = 1;
>      ctx->fhdr.chunk_offset       = ASF_FILE_HEADER_SIZE;
>      strncpy(ctx->fhdr.name, av_basename(s->url), FF_ARRAY_ELEMS(ctx->fhdr.name));
> @@ -340,6 +341,37 @@ static int argo_asf_write_trailer(AVFormatContext *s)
>      return 0;
>  }
>  
> +static const AVOption argo_asf_options[] = {
> +    {
> +        .name        = "version_major",
> +        .help        = "set file major version",
> +        .offset      = offsetof(ArgoASFContext, fhdr.version_major),
> +        .type        = AV_OPT_TYPE_INT,
> +        .default_val = {.i64 = 2},
> +        .min         = 0,
> +        .max         = UINT16_MAX,
> +        .flags       = AV_OPT_FLAG_ENCODING_PARAM
> +    },
> +    {
> +        .name        = "version_minor",
> +        .help        = "set file minor version",
> +        .offset      = offsetof(ArgoASFContext, fhdr.version_minor),
> +        .type        = AV_OPT_TYPE_INT,
> +        .default_val = {.i64 = 1},
> +        .min         = 0,
> +        .max         = UINT16_MAX,
> +        .flags       = AV_OPT_FLAG_ENCODING_PARAM
> +    },
> +    { NULL }
> +};
> +
> +static const AVClass argo_asf_muxer_class = {
> +    .class_name = "argo_asf_muxer",
> +    .item_name  = av_default_item_name,
> +    .option     = argo_asf_options,
> +    .version    = LIBAVUTIL_VERSION_INT
> +};
> +
>  AVOutputFormat ff_argo_asf_muxer = {
>      .name           = "argo_asf",
>      .long_name      = NULL_IF_CONFIG_SMALL("Argonaut Games ASF"),
> @@ -353,6 +385,7 @@ AVOutputFormat ff_argo_asf_muxer = {
>      .write_header   = argo_asf_write_header,
>      .write_packet   = argo_asf_write_packet,
>      .write_trailer  = argo_asf_write_trailer,
> +    .priv_class     = &argo_asf_muxer_class,
>      .priv_data_size = sizeof(ArgoASFContext)
>  };
>  #endif
> 
The version fields in the structure mimic the way the header is set up:
They are uint16_t. Yet the fields will be written with a pointer to int
(that's a consequence of using AV_OPT_TYPE_INT). This means that (most
likely) an unaligned pointer will be used to write one of them (most
likely the minor version) which is undefined behaviour. Furthermore this
won't work on big endian systems: When you write the major version, the
actual lower 16 bits (that matter) will be put into the minor version
(assuming no padding between the two versions) which will afterwards be
overwritten by the upper 16 bits of the minor version. The actual minor
version will be written into the upper 16 bits of the number of chunks
(again, assuming no padding). And finally, even on little-endian systems
it only works if the minor version is specified after the major version:
If you provide a major version via av_opt_set() (or via the ffmpeg
command line), it will zero the minor version even on little-endian
systems. In case no options are provided, this furthermore only works if
av_opt_set_defaults() applies options earlier in the AVOptions first. I
don't think such a dependency should exist.

This is of course another reason to use a separate context for the muxer.

- Andreas