[FFmpeg-devel] [PATCH 1/4] avformat/au: Store strings instead of pointers to strings in array

[Alexander Strasser]

Hi Andreas,

no objections for the patchset as a whole.

Just for the first in the series I have some questions.

The commit message is:

    avformat/au: Store strings instead of pointers to strings in array

This tells the what, but not the why.


On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> > ---
> >  libavformat/au.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavformat/au.c b/libavformat/au.c
> > index f92863e400..b419c9ed95 100644
> > --- a/libavformat/au.c
> > +++ b/libavformat/au.c
> > @@ -68,13 +68,13 @@ static int au_probe(const AVProbeData *p)
> >
> >  static int au_read_annotation(AVFormatContext *s, int size)
> >  {
> > -    static const char * keys[] = {
> > +    static const char keys[][7] = {
> >          "title",
> >          "artist",
> >          "album",
> >          "track",
> >          "genre",
> > -        NULL };
> > +    };

Sorry, I couldn't test myself but wouldn't just

    static const char * keys[] = {
        "title",
        "artist",
        "album",
        "track",
        "genre",
    };

have been the better choice with the same benefits?

I'm not sure without looking up the C standard and writing some
little test programs. From my guts I would say it's equivalent,
but the syntax is more convenient this way.

That's also another issue with the commit message, since I don't
think it is true that in your version strings are stored in the
array. No offense intended and I sure might be mistaken.


> >      AVIOContext *pb = s->pb;
> >      enum { PARSE_KEY, PARSE_VALUE, PARSE_FINISHED } state = PARSE_KEY;
> >      char c;
> > @@ -107,7 +107,7 @@ static int au_read_annotation(AVFormatContext *s, int size)
> >                      av_log(s, AV_LOG_ERROR, "Memory error while parsing AU metadata.\n");
> >                  } else {
> >                      av_bprint_init(&bprint, 64, AV_BPRINT_SIZE_UNLIMITED);
> > -                    for (i = 0; keys[i] != NULL && key != NULL; i++) {
> > +                    for (i = 0; i < FF_ARRAY_ELEMS(keys) && key != NULL; i++) {
> >                          if (av_strcasecmp(keys[i], key) == 0) {
> >                              av_dict_set(&(s->metadata), keys[i], value, AV_DICT_DONT_STRDUP_VAL);
> >                              av_freep(&key);
> > @@ -243,14 +243,13 @@ typedef struct AUContext {
> >
> >  static int au_get_annotations(AVFormatContext *s, char **buffer)
> >  {
> > -    static const char * keys[] = {
> > +    static const char keys[][7] = {
> >          "Title",
> >          "Artist",
> >          "Album",
> >          "Track",
> >          "Genre",
> > -        NULL };
> > -    int i;
> > +    };
> >      int cnt = 0;
> >      AVDictionary *m = s->metadata;
> >      AVDictionaryEntry *t = NULL;
> > @@ -258,7 +257,7 @@ static int au_get_annotations(AVFormatContext *s, char **buffer)
> >
> >      av_bprint_init(&bprint, 64, AV_BPRINT_SIZE_UNLIMITED);
> >
> > -    for (i = 0; keys[i] != NULL; i++) {
> > +    for (int i = 0; i < FF_ARRAY_ELEMS(keys); i++) {
> >          t = av_dict_get(m, keys[i], NULL, 0);
> >          if (t != NULL) {
> >              if (cnt++)
> >
> Will apply this patchset tomorrow unless there are objections.

No problem if I was to late to reply. Just got to read this mail now.


Best regards,
  Alexander