diff mbox series

[FFmpeg-devel,5/6] avformat/argo_asf: add name option

Message ID 20200808075914.2296555-5-zane@zanevaniperen.com
State Superseded
Headers show
Series [FFmpeg-devel,1/6] avformat/argo_asf: don't check or probe file version
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Zane van Iperen Aug. 8, 2020, 8:01 a.m. UTC
Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
---
 libavformat/argo_asf.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Aug. 8, 2020, 10:20 a.m. UTC | #1
Zane van Iperen:
> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
> ---
>  libavformat/argo_asf.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/argo_asf.c b/libavformat/argo_asf.c
> index 9845cb955b..3499519903 100644
> --- a/libavformat/argo_asf.c
> +++ b/libavformat/argo_asf.c
> @@ -69,6 +69,7 @@ typedef struct ArgoASFContext {
>      ArgoASFFileHeader   fhdr;
>      ArgoASFChunkHeader  ckhdr;
>      uint32_t            blocks_read;
> +    const char         *name;
>  } ArgoASFContext;
>  
>  #if CONFIG_ARGO_ASF_DEMUXER
> @@ -301,7 +302,10 @@ static int argo_asf_write_header(AVFormatContext *s)
>      /* 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));
> +    if (ctx->name)
> +        strncpy(ctx->fhdr.name, ctx->name, FF_ARRAY_ELEMS(ctx->fhdr.name));
> +    else
> +        strncpy(ctx->fhdr.name, av_basename(s->url), FF_ARRAY_ELEMS(ctx->fhdr.name));

FF_ARRAY_ELEMS() is actually inappropriate here: Use sizeof() directly.
No need to make a separate patch for it.

>  
>      ctx->ckhdr.num_blocks        = 0;
>      ctx->ckhdr.num_samples       = ASF_SAMPLE_COUNT;
> @@ -362,6 +366,14 @@ static const AVOption argo_asf_options[] = {
>          .max         = UINT16_MAX,
>          .flags       = AV_OPT_FLAG_ENCODING_PARAM
>      },
> +    {
> +        .name        = "name",
> +        .help        = "embedded file name (max 8 characters)",
> +        .offset      = offsetof(ArgoASFContext, name),
> +        .type        = AV_OPT_TYPE_STRING,
> +        .default_val = {.str = NULL},
> +        .flags       = AV_OPT_FLAG_ENCODING_PARAM
> +    },
>      { NULL }
>  };
I wonder whether it would not be better to set this via metadata (that
the demuxer could export so that it survives a roundtrip).

- Andreas
Zane van Iperen Aug. 8, 2020, 12:57 p.m. UTC | #2
On Sat, 8 Aug 2020 12:20:32 +0200
"Andreas Rheinhardt" <andreas.rheinhardt@gmail.com> wrote:

> >  #if CONFIG_ARGO_ASF_DEMUXER
> > @@ -301,7 +302,10 @@ static int argo_asf_write_header(AVFormatContext *s)
> >      /* 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));
> > +    if (ctx->name)
> > +        strncpy(ctx->fhdr.name, ctx->name, FF_ARRAY_ELEMS(ctx->fhdr.name));
> > +    else
> > +        strncpy(ctx->fhdr.name, av_basename(s->url), FF_ARRAY_ELEMS(ctx->fhdr.name));
> 
> FF_ARRAY_ELEMS() is actually inappropriate here: Use sizeof() directly.
Fixed.

> No need to make a separate patch for it.
Do you mean squash it into the previous one where I add the version
options?

> >
> >      ctx->ckhdr.num_blocks        = 0;
> >      ctx->ckhdr.num_samples       = ASF_SAMPLE_COUNT;
> > @@ -362,6 +366,14 @@ static const AVOption argo_asf_options[] = {
> >          .max         = UINT16_MAX,
> >          .flags       = AV_OPT_FLAG_ENCODING_PARAM
> >      },
> > +    {
> > +        .name        = "name",
> > +        .help        = "embedded file name (max 8 characters)",
> > +        .offset      = offsetof(ArgoASFContext, name),
> > +        .type        = AV_OPT_TYPE_STRING,
> > +        .default_val = {.str = NULL},
> > +        .flags       = AV_OPT_FLAG_ENCODING_PARAM
> > +    },
> >      { NULL }
> >  };
> I wonder whether it would not be better to set this via metadata (that
> the demuxer could export so that it survives a roundtrip).

Hmmm, possibly? I've only ever seen the field as the filename (minus
extension), so I'm not too sure how much value it has as metadata.

Zane
Andreas Rheinhardt Aug. 8, 2020, 1:01 p.m. UTC | #3
Zane van Iperen:
> On Sat, 8 Aug 2020 12:20:32 +0200
> "Andreas Rheinhardt" <andreas.rheinhardt@gmail.com> wrote:
> 
>>>  #if CONFIG_ARGO_ASF_DEMUXER
>>> @@ -301,7 +302,10 @@ static int argo_asf_write_header(AVFormatContext *s)
>>>      /* 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));
>>> +    if (ctx->name)
>>> +        strncpy(ctx->fhdr.name, ctx->name, FF_ARRAY_ELEMS(ctx->fhdr.name));
>>> +    else
>>> +        strncpy(ctx->fhdr.name, av_basename(s->url), FF_ARRAY_ELEMS(ctx->fhdr.name));
>>
>> FF_ARRAY_ELEMS() is actually inappropriate here: Use sizeof() directly.
> Fixed.
> 
>> No need to make a separate patch for it.
> Do you mean squash it into the previous one where I add the version
> options?
> 
No, I mean that you don't need to make a separate patch that only
changes FF_ARRAY_ELEMS to sizeof. You can directly change it in this
patch here.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/argo_asf.c b/libavformat/argo_asf.c
index 9845cb955b..3499519903 100644
--- a/libavformat/argo_asf.c
+++ b/libavformat/argo_asf.c
@@ -69,6 +69,7 @@  typedef struct ArgoASFContext {
     ArgoASFFileHeader   fhdr;
     ArgoASFChunkHeader  ckhdr;
     uint32_t            blocks_read;
+    const char         *name;
 } ArgoASFContext;
 
 #if CONFIG_ARGO_ASF_DEMUXER
@@ -301,7 +302,10 @@  static int argo_asf_write_header(AVFormatContext *s)
     /* 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));
+    if (ctx->name)
+        strncpy(ctx->fhdr.name, ctx->name, FF_ARRAY_ELEMS(ctx->fhdr.name));
+    else
+        strncpy(ctx->fhdr.name, av_basename(s->url), FF_ARRAY_ELEMS(ctx->fhdr.name));
 
     ctx->ckhdr.num_blocks        = 0;
     ctx->ckhdr.num_samples       = ASF_SAMPLE_COUNT;
@@ -362,6 +366,14 @@  static const AVOption argo_asf_options[] = {
         .max         = UINT16_MAX,
         .flags       = AV_OPT_FLAG_ENCODING_PARAM
     },
+    {
+        .name        = "name",
+        .help        = "embedded file name (max 8 characters)",
+        .offset      = offsetof(ArgoASFContext, name),
+        .type        = AV_OPT_TYPE_STRING,
+        .default_val = {.str = NULL},
+        .flags       = AV_OPT_FLAG_ENCODING_PARAM
+    },
     { NULL }
 };