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

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/au.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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 };
> +    };
>      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.

- Andreas

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

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.
> 
I thought the why to be obvious. See below.
> 
> On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.
> 
But it is true. The strings are stored directly in the array, so that
each array takes up 5 * 7 bytes. Before the change, the array itself
took up 5 * sizeof(char *). And on top of that comes the space for the
strings itself.

Storing the strings itself in the array instead of pointers to the
strings saves the space of the pointers, but it forces the shortest
string to the size of the longest string. Therefore it is worthwhile if
the average size differs from the longest size by less than sizeof(char
*); there is furthermore one exception to this rule: If one stores
pointers, different pointers may point to the same string (or to a part
of a larger string). In the case here, the strings itself are smaller
than sizeof(char *) on 64bit systems, so this alone shows that one saves
space.

Also notice that there are two more differences:
a) The earlier version consisted of an array of nonconst pointers to
const strings. Making the array entries const has been forgotten (it
would go like this: "static const char *const keys[]"). Given that
arrays are automatically nonmodifiable, this problem doesn't exist.
(Given that these arrays are static and given that the compiler can see
that they are not modified means that the compiler could infer that they
are const and put them in the appropriate section for const data.)
b) The actual access to the string is different: If the array contains
pointer to strings, then one has to read the array entry (containing the
pointer to the string) and pass it to av_strcasecmp() etc. If the array
contains the strings, then one just has to get the address of the array
entry (which coincides with the address of the string itself) and pass
it to av_strcasecmp() etc..

- Andreas

On 2020-07-19 23:38 +0200, Andreas Rheinhardt wrote:
> Alexander Strasser:
[...]
> >
> > On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
> >> Andreas Rheinhardt:
> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.
> >
> But it is true.

Yes, you're right. I read the array dimension the wrong way around :(

Sorry for the noise.


> The strings are stored directly in the array, so that
> each array takes up 5 * 7 bytes. Before the change, the array itself
> took up 5 * sizeof(char *). And on top of that comes the space for the
> strings itself.
>
> Storing the strings itself in the array instead of pointers to the
> strings saves the space of the pointers, but it forces the shortest
> string to the size of the longest string. Therefore it is worthwhile if
> the average size differs from the longest size by less than sizeof(char
> *); there is furthermore one exception to this rule: If one stores
> pointers, different pointers may point to the same string (or to a part
> of a larger string). In the case here, the strings itself are smaller
> than sizeof(char *) on 64bit systems, so this alone shows that one saves
> space.

So it's about 40 bytes on systems with 64 addresses, right?


> Also notice that there are two more differences:
> a) The earlier version consisted of an array of nonconst pointers to
> const strings. Making the array entries const has been forgotten (it
> would go like this: "static const char *const keys[]"). Given that
> arrays are automatically nonmodifiable, this problem doesn't exist.
> (Given that these arrays are static and given that the compiler can see
> that they are not modified means that the compiler could infer that they
> are const and put them in the appropriate section for const data.)
> b) The actual access to the string is different: If the array contains
> pointer to strings, then one has to read the array entry (containing the
> pointer to the string) and pass it to av_strcasecmp() etc. If the array
> contains the strings, then one just has to get the address of the array
> entry (which coincides with the address of the string itself) and pass
> it to av_strcasecmp() etc..

I agree the new version is technically superior.

It's also more rigid, but that should probably not matter much in
this specific case.

Still I think the savings are marginal, so I guess you did them
while looking into the code for fixing the things later in this
patch set.

Fair enough.

A small additional sentence in the commit message body, would have
been better IMHO:

   Save a bit of space and one layer of indirection.



Best regards,
  Alexander

Alexander Strasser:
> On 2020-07-19 23:38 +0200, Andreas Rheinhardt wrote:
>> Alexander Strasser:
> [...]
>>>
>>> On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
>>>> Andreas Rheinhardt:
>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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.
>>>
>> But it is true.
> 
> Yes, you're right. I read the array dimension the wrong way around :(
> 
> Sorry for the noise.
> 
> 
>> The strings are stored directly in the array, so that
>> each array takes up 5 * 7 bytes. Before the change, the array itself
>> took up 5 * sizeof(char *). And on top of that comes the space for the
>> strings itself.
>>
>> Storing the strings itself in the array instead of pointers to the
>> strings saves the space of the pointers, but it forces the shortest
>> string to the size of the longest string. Therefore it is worthwhile if
>> the average size differs from the longest size by less than sizeof(char
>> *); there is furthermore one exception to this rule: If one stores
>> pointers, different pointers may point to the same string (or to a part
>> of a larger string). In the case here, the strings itself are smaller
>> than sizeof(char *) on 64bit systems, so this alone shows that one saves
>> space.
> 
> So it's about 40 bytes on systems with 64 addresses, right?
> 
6 pointers amount to 48 bytes, but one also has to waste 4 bytes to make
the shorter strings as long as the longest.
> 
>> Also notice that there are two more differences:
>> a) The earlier version consisted of an array of nonconst pointers to
>> const strings. Making the array entries const has been forgotten (it
>> would go like this: "static const char *const keys[]"). Given that
>> arrays are automatically nonmodifiable, this problem doesn't exist.
>> (Given that these arrays are static and given that the compiler can see
>> that they are not modified means that the compiler could infer that they
>> are const and put them in the appropriate section for const data.)
>> b) The actual access to the string is different: If the array contains
>> pointer to strings, then one has to read the array entry (containing the
>> pointer to the string) and pass it to av_strcasecmp() etc. If the array
>> contains the strings, then one just has to get the address of the array
>> entry (which coincides with the address of the string itself) and pass
>> it to av_strcasecmp() etc..
> 
> I agree the new version is technically superior.
> 
> It's also more rigid, but that should probably not matter much in
> this specific case.
> 
Indeed, it is usually not suitable when one has too many strings.

> Still I think the savings are marginal, so I guess you did them
> while looking into the code for fixing the things later in this
> patch set.
> 
Yes.

- Andreas