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

Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
---
 libavformat/argo_asf.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Zane van Iperen:
> Signed-off-by: Zane van Iperen <zane@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

Hi Zane!


Am 8. August 2020 10:01:07 MESZ schrieb Zane van Iperen <zane@zanevaniperen.com>:
>Signed-off-by: Zane van Iperen <zane@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. */

Stupid question: Why are you adding options to override the file format version?

Normally those versions have implications on  the syntax and/or semantics of the files they are embedded in.

Am I misreading or misunderstanding something?


>     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
>-- 
>2.25.1

On Sat, 8 Aug 2020 12:15:01 +0200
"Andreas Rheinhardt" <andreas.rheinhardt@gmail.com> wrote:

> 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.

...I remember catching this when I had separate structures. I mustn't
have updated it when I merged them. *sigh*, well done Zane.

On Sat, 08 Aug 2020 13:05:28 +0200
"Alexander Strasser" <eclipse7@gmx.net> wrote:

> 
> Hi Zane!
> 
Hello!

> >@@ -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. */
> 
> Stupid question: Why are you adding options to override the file format version?
> 
> Normally those versions have implications on  the syntax and/or semantics of the files they are embedded in.
> 
> Am I misreading or misunderstanding something?
> 

You're right to think that, that's what I originally thought too.
When researching the file format, I investigated several different file
versions and their structure was identical.

I think it's really only used as some kind of psuedo-identifier that
tells which game the file came from, and I'm pretty sure some of the
games check the version too (hence the options).

I'd be happy to be proven wrong, but until other files surface I think
allowing the user to specify the version is fine.

Zane

Am 8. August 2020 14:52:02 MESZ schrieb Zane van Iperen <zane@zanevaniperen.com>:
>On Sat, 08 Aug 2020 13:05:28 +0200
>"Alexander Strasser" <eclipse7@gmx.net> wrote:
>
>> >@@ -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. */
>> 
>> Stupid question: Why are you adding options to override the file
>format version?
>> 
>> Normally those versions have implications on  the syntax and/or
>semantics of the files they are embedded in.
>> 
>> Am I misreading or misunderstanding something?
>> 
>
>You're right to think that, that's what I originally thought too.
>When researching the file format, I investigated several different file
>versions and their structure was identical.
>
>I think it's really only used as some kind of psuedo-identifier that
>tells which game the file came from, and I'm pretty sure some of the
>games check the version too (hence the options).
>
>I'd be happy to be proven wrong, but until other files surface I think
>allowing the user to specify the version is fine.

I see. Maybe it's not the version of the format, but has implications of what can be inside. Anyway I didn't look at any files, so just some wild guessing.

I would advise to use the verb override instead of set in the option description. I think it's less ambiguous. Like more expectations "you are doing this on your own" and less "FFmpeg will use Version 2.4 of the format now".


  Alexander