| Message ID | 20200812204940.32100-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avfilter/formats: Remove ff_make_formatu64_list() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/configure | warning | Failed to apply patch |
Andreas Rheinhardt (12020-08-12): > When one merges two AVFilterChannelLayouts structs, there is no need to > allocate a new one. Instead one can reuse one of the two given ones. > If one does this, one also doesn't need to update the references of the > AVFilterChannelLayouts that is reused. Therefore this commit reuses the > structure with the higher refcount. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This can of course be applied independently of 7-21. > > libavfilter/formats.c | 48 +++++++++++++++++++------------------------ > 1 file changed, 21 insertions(+), 27 deletions(-) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index 00d050e439..2d33dd7afe 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -48,13 +48,13 @@ do { \ > av_freep(&a); \ > } while (0) > > -#define MERGE_REF(ret, a, fmts, type, fail) \ > +#define MERGE_REF(ret, a, fmts, type, fail_statement) \ > do { \ > type ***tmp; \ > \ > if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount, \ > sizeof(*tmp)))) \ > - goto fail; \ > + { fail_statement } \ > ret->refs = tmp; \ > MERGE_REF_NO_ALLOC(ret, a, fmts); \ > } while (0) > @@ -157,10 +157,10 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a, > if (a->nb_formats && b->nb_formats) { > MERGE_FORMATS(ret, a, b, formats, nb_formats, AVFilterFormats, fail); > } else if (a->nb_formats) { > - MERGE_REF(a, b, formats, AVFilterFormats, fail); > + MERGE_REF(a, b, formats, AVFilterFormats, return NULL;); > ret = a; > } else { > - MERGE_REF(b, a, formats, AVFilterFormats, fail); > + MERGE_REF(b, a, formats, AVFilterFormats, return NULL;); > ret = b; > } > > @@ -177,7 +177,7 @@ fail: > AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, > AVFilterChannelLayouts *b) > { > - AVFilterChannelLayouts *ret = NULL; > + uint64_t *channel_layouts; > unsigned a_all = a->all_layouts + a->all_counts; > unsigned b_all = b->all_layouts + b->all_counts; > int ret_max, ret_nb = 0, i, j, round; > @@ -201,15 +201,13 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, > return NULL; > b->nb_channel_layouts = j; > } > - MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, fail); > + MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, return NULL;); > return b; > } > > ret_max = a->nb_channel_layouts + b->nb_channel_layouts; > - if (!(ret = av_mallocz(sizeof(*ret))) || > - !(ret->channel_layouts = av_malloc_array(ret_max, > - sizeof(*ret->channel_layouts)))) > - goto fail; > + if (!(channel_layouts = av_malloc_array(ret_max, sizeof(channel_layouts)))) sizeof(*channel_layouts) > + return NULL; > > /* a[known] intersect b[known] */ > for (i = 0; i < a->nb_channel_layouts; i++) { > @@ -217,7 +215,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, > continue; > for (j = 0; j < b->nb_channel_layouts; j++) { > if (a->channel_layouts[i] == b->channel_layouts[j]) { > - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; > + channel_layouts[ret_nb++] = a->channel_layouts[i]; > a->channel_layouts[i] = b->channel_layouts[j] = 0; > } > } > @@ -232,7 +230,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, > bfmt = FF_COUNT2LAYOUT(av_get_channel_layout_nb_channels(fmt)); > for (j = 0; j < b->nb_channel_layouts; j++) > if (b->channel_layouts[j] == bfmt) > - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; > + channel_layouts[ret_nb++] = a->channel_layouts[i]; > } > /* 1st round: swap to prepare 2nd round; 2nd round: put it back */ > FFSWAP(AVFilterChannelLayouts *, a, b); > @@ -243,27 +241,23 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, > continue; > for (j = 0; j < b->nb_channel_layouts; j++) > if (a->channel_layouts[i] == b->channel_layouts[j]) > - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; > + channel_layouts[ret_nb++] = a->channel_layouts[i]; > } > > - ret->nb_channel_layouts = ret_nb; > - if (!ret->nb_channel_layouts) > + if (!ret_nb) > goto fail; > > - ret->refs = av_realloc_array(NULL, a->refcount + b->refcount, > - sizeof(*ret->refs)); > - if (!ret->refs) > - goto fail; > - MERGE_REF_NO_ALLOC(ret, a, channel_layouts); > - MERGE_REF_NO_ALLOC(ret, b, channel_layouts); > - return ret; > + if (a->refcount > b->refcount) > + FFSWAP(AVFilterChannelLayouts *, a, b); I do not think it is just as simple as that: if you swap, then b->refs points to the references of a and a->refs points to the references of b. Then the MERGE_REF() below will update a->refs, i.e. the references of b, which are already valid, and not b->refs, which need to. Better leave this optimization for later. Especially if we consider using a linked list as I suggested. > + > + MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, goto fail;); > + av_freep(&b->channel_layouts); > + b->channel_layouts = channel_layouts; > + b->nb_channel_layouts = ret_nb; > + return b; > > fail: > - if (ret) { > - av_assert1(!ret->refs); > - av_freep(&ret->channel_layouts); > - av_freep(&ret); > - } > + av_free(channel_layouts); > return NULL; > } LGTM apart from that. Regards,
Nicolas George: > Andreas Rheinhardt (12020-08-12): >> When one merges two AVFilterChannelLayouts structs, there is no need to >> allocate a new one. Instead one can reuse one of the two given ones. >> If one does this, one also doesn't need to update the references of the >> AVFilterChannelLayouts that is reused. Therefore this commit reuses the >> structure with the higher refcount. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> This can of course be applied independently of 7-21. >> >> libavfilter/formats.c | 48 +++++++++++++++++++------------------------ >> 1 file changed, 21 insertions(+), 27 deletions(-) >> >> diff --git a/libavfilter/formats.c b/libavfilter/formats.c >> index 00d050e439..2d33dd7afe 100644 >> --- a/libavfilter/formats.c >> +++ b/libavfilter/formats.c >> @@ -48,13 +48,13 @@ do { \ >> av_freep(&a); \ >> } while (0) >> >> -#define MERGE_REF(ret, a, fmts, type, fail) \ >> +#define MERGE_REF(ret, a, fmts, type, fail_statement) \ >> do { \ >> type ***tmp; \ >> \ >> if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount, \ >> sizeof(*tmp)))) \ >> - goto fail; \ >> + { fail_statement } \ >> ret->refs = tmp; \ >> MERGE_REF_NO_ALLOC(ret, a, fmts); \ >> } while (0) >> @@ -157,10 +157,10 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a, >> if (a->nb_formats && b->nb_formats) { >> MERGE_FORMATS(ret, a, b, formats, nb_formats, AVFilterFormats, fail); >> } else if (a->nb_formats) { >> - MERGE_REF(a, b, formats, AVFilterFormats, fail); >> + MERGE_REF(a, b, formats, AVFilterFormats, return NULL;); >> ret = a; >> } else { >> - MERGE_REF(b, a, formats, AVFilterFormats, fail); >> + MERGE_REF(b, a, formats, AVFilterFormats, return NULL;); >> ret = b; >> } >> >> @@ -177,7 +177,7 @@ fail: >> AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, >> AVFilterChannelLayouts *b) >> { >> - AVFilterChannelLayouts *ret = NULL; >> + uint64_t *channel_layouts; >> unsigned a_all = a->all_layouts + a->all_counts; >> unsigned b_all = b->all_layouts + b->all_counts; >> int ret_max, ret_nb = 0, i, j, round; >> @@ -201,15 +201,13 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, >> return NULL; >> b->nb_channel_layouts = j; >> } >> - MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, fail); >> + MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, return NULL;); >> return b; >> } >> >> ret_max = a->nb_channel_layouts + b->nb_channel_layouts; >> - if (!(ret = av_mallocz(sizeof(*ret))) || >> - !(ret->channel_layouts = av_malloc_array(ret_max, >> - sizeof(*ret->channel_layouts)))) >> - goto fail; > >> + if (!(channel_layouts = av_malloc_array(ret_max, sizeof(channel_layouts)))) > > sizeof(*channel_layouts) True. Fixed locally. > >> + return NULL; >> >> /* a[known] intersect b[known] */ >> for (i = 0; i < a->nb_channel_layouts; i++) { >> @@ -217,7 +215,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, >> continue; >> for (j = 0; j < b->nb_channel_layouts; j++) { >> if (a->channel_layouts[i] == b->channel_layouts[j]) { >> - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; >> + channel_layouts[ret_nb++] = a->channel_layouts[i]; >> a->channel_layouts[i] = b->channel_layouts[j] = 0; >> } >> } >> @@ -232,7 +230,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, >> bfmt = FF_COUNT2LAYOUT(av_get_channel_layout_nb_channels(fmt)); >> for (j = 0; j < b->nb_channel_layouts; j++) >> if (b->channel_layouts[j] == bfmt) >> - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; >> + channel_layouts[ret_nb++] = a->channel_layouts[i]; >> } >> /* 1st round: swap to prepare 2nd round; 2nd round: put it back */ >> FFSWAP(AVFilterChannelLayouts *, a, b); >> @@ -243,27 +241,23 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, >> continue; >> for (j = 0; j < b->nb_channel_layouts; j++) >> if (a->channel_layouts[i] == b->channel_layouts[j]) >> - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; >> + channel_layouts[ret_nb++] = a->channel_layouts[i]; >> } >> >> - ret->nb_channel_layouts = ret_nb; >> - if (!ret->nb_channel_layouts) >> + if (!ret_nb) >> goto fail; >> >> - ret->refs = av_realloc_array(NULL, a->refcount + b->refcount, >> - sizeof(*ret->refs)); >> - if (!ret->refs) >> - goto fail; >> - MERGE_REF_NO_ALLOC(ret, a, channel_layouts); >> - MERGE_REF_NO_ALLOC(ret, b, channel_layouts); >> - return ret; > >> + if (a->refcount > b->refcount) >> + FFSWAP(AVFilterChannelLayouts *, a, b); > > I do not think it is just as simple as that: if you swap, then b->refs > points to the references of a and a->refs points to the references of b. > Then the MERGE_REF() below will update a->refs, i.e. the references of > b, which are already valid, and not b->refs, which need to. No, I just swap the pointers, not the structures themselves. It just ensures a->refcount <= b->refcount in the following code. (The same swapping btw already happens at the beginning of the function.) > > Better leave this optimization for later. Especially if we consider > using a linked list as I suggested. > >> + >> + MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, goto fail;); >> + av_freep(&b->channel_layouts); >> + b->channel_layouts = channel_layouts; >> + b->nb_channel_layouts = ret_nb; >> + return b; >> >> fail: >> - if (ret) { >> - av_assert1(!ret->refs); >> - av_freep(&ret->channel_layouts); >> - av_freep(&ret); >> - } >> + av_free(channel_layouts); >> return NULL; >> } > > LGTM apart from that. > > Regards, >
Andreas Rheinhardt (12020-08-13): > No, I just swap the pointers, not the structures themselves. It just > ensures a->refcount <= b->refcount in the following code. (The same > swapping btw already happens at the beginning of the function.) Oh, yes, my bad. I thought you wanted to reuse the formats array, but it is not possible for channel layouts. Then please go ahead. Regards,
diff --git a/libavfilter/formats.c b/libavfilter/formats.c index 00d050e439..2d33dd7afe 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -48,13 +48,13 @@ do { \ av_freep(&a); \ } while (0) -#define MERGE_REF(ret, a, fmts, type, fail) \ +#define MERGE_REF(ret, a, fmts, type, fail_statement) \ do { \ type ***tmp; \ \ if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount, \ sizeof(*tmp)))) \ - goto fail; \ + { fail_statement } \ ret->refs = tmp; \ MERGE_REF_NO_ALLOC(ret, a, fmts); \ } while (0) @@ -157,10 +157,10 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a, if (a->nb_formats && b->nb_formats) { MERGE_FORMATS(ret, a, b, formats, nb_formats, AVFilterFormats, fail); } else if (a->nb_formats) { - MERGE_REF(a, b, formats, AVFilterFormats, fail); + MERGE_REF(a, b, formats, AVFilterFormats, return NULL;); ret = a; } else { - MERGE_REF(b, a, formats, AVFilterFormats, fail); + MERGE_REF(b, a, formats, AVFilterFormats, return NULL;); ret = b; } @@ -177,7 +177,7 @@ fail: AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, AVFilterChannelLayouts *b) { - AVFilterChannelLayouts *ret = NULL; + uint64_t *channel_layouts; unsigned a_all = a->all_layouts + a->all_counts; unsigned b_all = b->all_layouts + b->all_counts; int ret_max, ret_nb = 0, i, j, round; @@ -201,15 +201,13 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, return NULL; b->nb_channel_layouts = j; } - MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, fail); + MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, return NULL;); return b; } ret_max = a->nb_channel_layouts + b->nb_channel_layouts; - if (!(ret = av_mallocz(sizeof(*ret))) || - !(ret->channel_layouts = av_malloc_array(ret_max, - sizeof(*ret->channel_layouts)))) - goto fail; + if (!(channel_layouts = av_malloc_array(ret_max, sizeof(channel_layouts)))) + return NULL; /* a[known] intersect b[known] */ for (i = 0; i < a->nb_channel_layouts; i++) { @@ -217,7 +215,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, continue; for (j = 0; j < b->nb_channel_layouts; j++) { if (a->channel_layouts[i] == b->channel_layouts[j]) { - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; + channel_layouts[ret_nb++] = a->channel_layouts[i]; a->channel_layouts[i] = b->channel_layouts[j] = 0; } } @@ -232,7 +230,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, bfmt = FF_COUNT2LAYOUT(av_get_channel_layout_nb_channels(fmt)); for (j = 0; j < b->nb_channel_layouts; j++) if (b->channel_layouts[j] == bfmt) - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; + channel_layouts[ret_nb++] = a->channel_layouts[i]; } /* 1st round: swap to prepare 2nd round; 2nd round: put it back */ FFSWAP(AVFilterChannelLayouts *, a, b); @@ -243,27 +241,23 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, continue; for (j = 0; j < b->nb_channel_layouts; j++) if (a->channel_layouts[i] == b->channel_layouts[j]) - ret->channel_layouts[ret_nb++] = a->channel_layouts[i]; + channel_layouts[ret_nb++] = a->channel_layouts[i]; } - ret->nb_channel_layouts = ret_nb; - if (!ret->nb_channel_layouts) + if (!ret_nb) goto fail; - ret->refs = av_realloc_array(NULL, a->refcount + b->refcount, - sizeof(*ret->refs)); - if (!ret->refs) - goto fail; - MERGE_REF_NO_ALLOC(ret, a, channel_layouts); - MERGE_REF_NO_ALLOC(ret, b, channel_layouts); - return ret; + if (a->refcount > b->refcount) + FFSWAP(AVFilterChannelLayouts *, a, b); + + MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, goto fail;); + av_freep(&b->channel_layouts); + b->channel_layouts = channel_layouts; + b->nb_channel_layouts = ret_nb; + return b; fail: - if (ret) { - av_assert1(!ret->refs); - av_freep(&ret->channel_layouts); - av_freep(&ret); - } + av_free(channel_layouts); return NULL; }
When one merges two AVFilterChannelLayouts structs, there is no need to allocate a new one. Instead one can reuse one of the two given ones. If one does this, one also doesn't need to update the references of the AVFilterChannelLayouts that is reused. Therefore this commit reuses the structure with the higher refcount. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- This can of course be applied independently of 7-21. libavfilter/formats.c | 48 +++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 27 deletions(-)