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

[Andreas Rheinhardt]

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