Message ID | 20200713194211.30244-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Commit | 8d3556b7a3f89f5dd621c3359f8690cf38796db7 |
Headers | show |
Series | [FFmpeg-devel,1/4] avformat/au: Store strings instead of pointers to strings in array | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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
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++)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/au.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)