Message ID | 20200808140202.586-5-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/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
Andreas Rheinhardt (12020-08-08): > Now that the output's refs-array is only allocated once, it is NULL in > any error case and therefore needn't be freed at all. Furthermore, it is > unnecessary to av_freep(&ptr) when ptr == NULL. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavfilter/formats.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index ae44006dfe..e452fd0408 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -141,10 +141,9 @@ AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b, > return ret; > fail: > if (ret) { > - av_freep(&ret->refs); It seems fragile. We could easily unmake the change that guarantees that refs is NULL, or use fail for another exit later. Maybe add an av_assert1()? > av_freep(&ret->formats); > + av_freep(&ret); > } > - av_freep(&ret); This one ok, of course. > return NULL; > } > > @@ -168,10 +167,9 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a, > return ret; > fail: > if (ret) { > - av_freep(&ret->refs); > av_freep(&ret->formats); > + av_freep(&ret); > } > - av_freep(&ret); > return NULL; > } > > @@ -261,10 +259,9 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, > > fail: > if (ret) { > - av_freep(&ret->refs); > av_freep(&ret->channel_layouts); > + av_freep(&ret); > } > - av_freep(&ret); > return NULL; > } Regards,
Nicolas George: > Andreas Rheinhardt (12020-08-08): >> Now that the output's refs-array is only allocated once, it is NULL in >> any error case and therefore needn't be freed at all. Furthermore, it is >> unnecessary to av_freep(&ptr) when ptr == NULL. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavfilter/formats.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/libavfilter/formats.c b/libavfilter/formats.c >> index ae44006dfe..e452fd0408 100644 >> --- a/libavfilter/formats.c >> +++ b/libavfilter/formats.c >> @@ -141,10 +141,9 @@ AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b, >> return ret; >> fail: >> if (ret) { > >> - av_freep(&ret->refs); > > It seems fragile. We could easily unmake the change that guarantees that > refs is NULL, or use fail for another exit later. Maybe add an > av_assert1()? > Fixed locally. - Andreas PS: Is the order of the formats/channel_layouts arrays actually important?
Andreas Rheinhardt (12020-08-08):
> PS: Is the order of the formats/channel_layouts arrays actually important?
IIRC, there are parts of the code that selects best formats and put them
in the front of the list. I need to dig back into all this to refresh my
memory, it is complex and it will need to be simplified before it can be
extended for subtitles or data packets.
Regards,
Nicolas George: > Andreas Rheinhardt (12020-08-08): >> PS: Is the order of the formats/channel_layouts arrays actually important? > > IIRC, there are parts of the code that selects best formats and put them > in the front of the list. I need to dig back into all this to refresh my > memory, it is complex and it will need to be simplified before it can be > extended for subtitles or data packets. > I was thinking about this part of the MERGE_FORMATS macro: for (i = 0; i < a->nb; i++) for (j = 0; j < b->nb; j++) if (a->fmts[i] == b->fmts[j]) { if(k >= FFMIN(a->nb, b->nb)){ av_log(NULL, AV_LOG_ERROR, "Duplicate formats in %s detected\n", __FUNCTION__); av_free(ret->fmts); av_free(ret); return NULL; } ret->fmts[k++] = a->fmts[i]; } Said check is not sufficient to ensure that there are no duplicates in ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345 as an additional safety check to ensure that one does not write beyond the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal could also be achieved in a simpler way: If a->nb <= b->nb, one could simply rewrite the above to for (i = 0; i < a->nb; i++) for (j = 0; j < b->nb; j++) if (a->fmts[i] == b->fmts[j]) { ret->fmts[k++] = a->fmts[i]; break; } This ensures that every entry of a will end up at most once in ret. And if one were simply allowed to switch a and b, one could ensure a->nb <= b->nb easily. ff_merge_channel_layouts() already does something similar. As a next step one could then even reuse the a->fmts array for ret->fmts (or simply reuse a altogether); this is possible because k <= i at the beginning of every iteration of the outer loop. Notice that if there is a total ordering of all the formats and if both a->fmts and b->fmts are ordered according to this ordering, then ret->fmts will also ordered in that way (regardless of whether one swaps or not). - Andreas
Andreas Rheinhardt (12020-08-08): > I was thinking about this part of the MERGE_FORMATS macro: > > for (i = 0; i < a->nb; i++) > for (j = 0; j < b->nb; j++) > if (a->fmts[i] == b->fmts[j]) { > if(k >= FFMIN(a->nb, b->nb)){ > av_log(NULL, AV_LOG_ERROR, "Duplicate formats in > %s detected\n", __FUNCTION__); > av_free(ret->fmts); > av_free(ret); > return NULL; > } > ret->fmts[k++] = a->fmts[i]; > } > > Said check is not sufficient to ensure that there are no duplicates in > ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345 > as an additional safety check to ensure that one does not write beyond > the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal > could also be achieved in a simpler way: If a->nb <= b->nb, one could > simply rewrite the above to This is only a protection against internal bugs, since lists should not contain duplicates in the first place. As such it should have been an assert. If we no longer need it, it would probably be better to make a conditional complete test for duplicates somewhere. > for (i = 0; i < a->nb; i++) > for (j = 0; j < b->nb; j++) > if (a->fmts[i] == b->fmts[j]) { > ret->fmts[k++] = a->fmts[i]; > break; > } > > This ensures that every entry of a will end up at most once in ret. And > if one were simply allowed to switch a and b, one could ensure a->nb <= > b->nb easily. ff_merge_channel_layouts() already does something similar. I noticed something similar when refreshing my memory about this. Indeed, breaking out of the loop when a match is found would make the same guarantee, and a nice optimization for a quadratic algorithm. Now, I can confirm, the order of formats in the array is not relevant for the rest of the negotiation. > As a next step one could then even reuse the a->fmts array for ret->fmts > (or simply reuse a altogether); this is possible because k <= i at the > beginning of every iteration of the outer loop. Indeed. > Notice that if there is a total ordering of all the formats and if both These are mostly numbers, we can choose to use the numeric ordering or a variant. > a->fmts and b->fmts are ordered according to this ordering, then > ret->fmts will also ordered in that way (regardless of whether one swaps > or not). Even better: if the lists are ordered, then we can switch from a quadratic algorithm to a linear one. Good idea. Regards,
Nicolas George: > Andreas Rheinhardt (12020-08-08): >> I was thinking about this part of the MERGE_FORMATS macro: >> >> for (i = 0; i < a->nb; i++) >> for (j = 0; j < b->nb; j++) >> if (a->fmts[i] == b->fmts[j]) { >> if(k >= FFMIN(a->nb, b->nb)){ >> av_log(NULL, AV_LOG_ERROR, "Duplicate formats in >> %s detected\n", __FUNCTION__); >> av_free(ret->fmts); >> av_free(ret); >> return NULL; >> } >> ret->fmts[k++] = a->fmts[i]; >> } >> >> Said check is not sufficient to ensure that there are no duplicates in >> ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345 >> as an additional safety check to ensure that one does not write beyond >> the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal >> could also be achieved in a simpler way: If a->nb <= b->nb, one could >> simply rewrite the above to > > This is only a protection against internal bugs, since lists should not > contain duplicates in the first place. As such it should have been an > assert. If we no longer need it, it would probably be better to make a > conditional complete test for duplicates somewhere. > >> for (i = 0; i < a->nb; i++) >> for (j = 0; j < b->nb; j++) >> if (a->fmts[i] == b->fmts[j]) { >> ret->fmts[k++] = a->fmts[i]; >> break; >> } >> >> This ensures that every entry of a will end up at most once in ret. And >> if one were simply allowed to switch a and b, one could ensure a->nb <= >> b->nb easily. ff_merge_channel_layouts() already does something similar. > > I noticed something similar when refreshing my memory about this. > Indeed, breaking out of the loop when a match is found would make the > same guarantee, and a nice optimization for a quadratic algorithm. > > Now, I can confirm, the order of formats in the array is not relevant > for the rest of the negotiation. > Sure about that? The filter-framerate-up test fails (the output pixel format is now YUV9 instead of I420) when I switch a and b in MERGE_FORMATS if b->nb < a->nb. What you said seems to only apply to sample rates, not format negotiations. One can of course still avoid the allocations, if one is either willing to keep the bigger of the two formats buffers or if one reallocates the buffer to the correct (usually even smaller) size afterwards. I'll go with the latter. - Andreas
Andreas Rheinhardt (12020-08-13): > Sure about that? The filter-framerate-up test fails (the output pixel > format is now YUV9 instead of I420) when I switch a and b in > MERGE_FORMATS if b->nb < a->nb. What you said seems to only apply to > sample rates, not format negotiations. Well, there is one aspect of the order that matters: if no stronger criterion applies and still several formats are possible, the first one will be selected. For most cases, it does not matter, because the input data will constrain the type, or possibly the output codec. But here, the input is testsrc2 and the output is framecrc, they support many formats. It could be a good idea to define some kind of "best" pixel formats when there is choice like that, but there is no urgency. More importantly, these test should be fixed to specify -pix_fmt and disable automatic conversions. I will try to have a look. Regards,
diff --git a/libavfilter/formats.c b/libavfilter/formats.c index ae44006dfe..e452fd0408 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -141,10 +141,9 @@ AVFilterFormats *ff_merge_formats(AVFilterFormats *a, AVFilterFormats *b, return ret; fail: if (ret) { - av_freep(&ret->refs); av_freep(&ret->formats); + av_freep(&ret); } - av_freep(&ret); return NULL; } @@ -168,10 +167,9 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a, return ret; fail: if (ret) { - av_freep(&ret->refs); av_freep(&ret->formats); + av_freep(&ret); } - av_freep(&ret); return NULL; } @@ -261,10 +259,9 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a, fail: if (ret) { - av_freep(&ret->refs); av_freep(&ret->channel_layouts); + av_freep(&ret); } - av_freep(&ret); return NULL; }
Now that the output's refs-array is only allocated once, it is NULL in any error case and therefore needn't be freed at all. Furthermore, it is unnecessary to av_freep(&ptr) when ptr == NULL. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavfilter/formats.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)