| Message ID | 20220314210603.23870-4-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | ea887ef8761e44bdef9e2a3daea08a0d2dcb8fef |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avfilter/x86/vf_blend: use unaligned movs for output | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
lgtm
On 3/14/2022 6:06 PM, Marton Balint wrote: > Output buffer alignment might be different to ZIMG_ALIGNMENT or it may not be > aligned at all if a downstream filter (e.g. vf_pad) intentionally misaligns it. av_frame_get_buffer() align parameter is used to align linesizes, not buffers. av_malloc() has a hardcoded alignment defined at compile time based on configure settings. If what you need is aligned data pointers, you should either use the trick i used in the libdav1d wrapper, or change av_malloc() in order to use av_cpu_max_align(). > > Or maybe we should unconditionally always allocate output with > av_frame_get_buffer() instead of ff_get_video_buffer()? > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavfilter/vf_zscale.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c > index ceefc95224..2061e38bcc 100644 > --- a/libavfilter/vf_zscale.c > +++ b/libavfilter/vf_zscale.c > @@ -632,7 +632,7 @@ static int graphs_build(AVFrame *in, AVFrame *out, const AVPixFmtDescriptor *des > return 0; > } > > -static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame **frame) > +static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame **frame, int needs_copy) > { > AVFrame *aligned = NULL; > int ret = 0, plane, planes; > @@ -654,10 +654,10 @@ static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame **frame) > if ((ret = av_frame_get_buffer(aligned, ZIMG_ALIGNMENT)) < 0) > goto fail; > > - if ((ret = av_frame_copy(aligned, *frame)) < 0) > + if (needs_copy && (ret = av_frame_copy(aligned, *frame)) < 0) > goto fail; > > - if ((ret = av_frame_copy_props(aligned, *frame)) < 0) > + if (needs_copy && (ret = av_frame_copy_props(aligned, *frame)) < 0) > goto fail; > > av_frame_free(frame); > @@ -786,9 +786,12 @@ static int filter_frame(AVFilterLink *link, AVFrame *in) > goto fail; > } > > + if ((ret = realign_frame(odesc, &out, 0)) < 0) > + goto fail; > + > av_frame_copy_props(out, in); > > - if ((ret = realign_frame(desc, &in)) < 0) > + if ((ret = realign_frame(desc, &in, 1)) < 0) > goto fail; > > snprintf(buf, sizeof(buf)-1, "%d", outlink->w);
On 3/16/2022 5:52 PM, James Almer wrote: > On 3/14/2022 6:06 PM, Marton Balint wrote: >> Output buffer alignment might be different to ZIMG_ALIGNMENT or it may >> not be >> aligned at all if a downstream filter (e.g. vf_pad) intentionally >> misaligns it. > > av_frame_get_buffer() align parameter is used to align linesizes, not > buffers. av_malloc() has a hardcoded alignment defined at compile time > based on configure settings. > > If what you need is aligned data pointers, you should either use the > trick i used in the libdav1d wrapper, or change av_malloc() in order to > use av_cpu_max_align(). > >> >> Or maybe we should unconditionally always allocate output with >> av_frame_get_buffer() instead of ff_get_video_buffer()? Also, ff_get_video_buffer() can be made to accept an align parameter, much like av_frame_get_buffer(). The latter (and av_frame_make_writable) should not be used in lavfi since you're losing the benefits from the AVFilterLink buffer pool. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavfilter/vf_zscale.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c >> index ceefc95224..2061e38bcc 100644 >> --- a/libavfilter/vf_zscale.c >> +++ b/libavfilter/vf_zscale.c >> @@ -632,7 +632,7 @@ static int graphs_build(AVFrame *in, AVFrame *out, >> const AVPixFmtDescriptor *des >> return 0; >> } >> -static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame >> **frame) >> +static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame >> **frame, int needs_copy) >> { >> AVFrame *aligned = NULL; >> int ret = 0, plane, planes; >> @@ -654,10 +654,10 @@ static int realign_frame(const >> AVPixFmtDescriptor *desc, AVFrame **frame) >> if ((ret = av_frame_get_buffer(aligned, ZIMG_ALIGNMENT)) >> < 0) >> goto fail; >> - if ((ret = av_frame_copy(aligned, *frame)) < 0) >> + if (needs_copy && (ret = av_frame_copy(aligned, *frame)) >> < 0) >> goto fail; >> - if ((ret = av_frame_copy_props(aligned, *frame)) < 0) >> + if (needs_copy && (ret = av_frame_copy_props(aligned, >> *frame)) < 0) >> goto fail; >> av_frame_free(frame); >> @@ -786,9 +786,12 @@ static int filter_frame(AVFilterLink *link, >> AVFrame *in) >> goto fail; >> } >> + if ((ret = realign_frame(odesc, &out, 0)) < 0) >> + goto fail; >> + >> av_frame_copy_props(out, in); >> - if ((ret = realign_frame(desc, &in)) < 0) >> + if ((ret = realign_frame(desc, &in, 1)) < 0) >> goto fail; >> snprintf(buf, sizeof(buf)-1, "%d", outlink->w);
On Wed, 16 Mar 2022, James Almer wrote: > On 3/14/2022 6:06 PM, Marton Balint wrote: >> Output buffer alignment might be different to ZIMG_ALIGNMENT or it may not >> be >> aligned at all if a downstream filter (e.g. vf_pad) intentionally >> misaligns it. > > av_frame_get_buffer() align parameter is used to align linesizes, not > buffers. av_malloc() has a hardcoded alignment defined at compile time based > on configure settings. Good point. > > If what you need is aligned data pointers, you should either use the trick i > used in the libdav1d wrapper, or change av_malloc() in order to use > av_cpu_max_align(). Actually av_cpu_max_align() also depends on compile time, so your trick is the only way to ensure ZIMG alignment as far as I see... Regards, Marton
diff --git a/libavfilter/vf_zscale.c b/libavfilter/vf_zscale.c index ceefc95224..2061e38bcc 100644 --- a/libavfilter/vf_zscale.c +++ b/libavfilter/vf_zscale.c @@ -632,7 +632,7 @@ static int graphs_build(AVFrame *in, AVFrame *out, const AVPixFmtDescriptor *des return 0; } -static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame **frame) +static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame **frame, int needs_copy) { AVFrame *aligned = NULL; int ret = 0, plane, planes; @@ -654,10 +654,10 @@ static int realign_frame(const AVPixFmtDescriptor *desc, AVFrame **frame) if ((ret = av_frame_get_buffer(aligned, ZIMG_ALIGNMENT)) < 0) goto fail; - if ((ret = av_frame_copy(aligned, *frame)) < 0) + if (needs_copy && (ret = av_frame_copy(aligned, *frame)) < 0) goto fail; - if ((ret = av_frame_copy_props(aligned, *frame)) < 0) + if (needs_copy && (ret = av_frame_copy_props(aligned, *frame)) < 0) goto fail; av_frame_free(frame); @@ -786,9 +786,12 @@ static int filter_frame(AVFilterLink *link, AVFrame *in) goto fail; } + if ((ret = realign_frame(odesc, &out, 0)) < 0) + goto fail; + av_frame_copy_props(out, in); - if ((ret = realign_frame(desc, &in)) < 0) + if ((ret = realign_frame(desc, &in, 1)) < 0) goto fail; snprintf(buf, sizeof(buf)-1, "%d", outlink->w);
Output buffer alignment might be different to ZIMG_ALIGNMENT or it may not be aligned at all if a downstream filter (e.g. vf_pad) intentionally misaligns it. Or maybe we should unconditionally always allocate output with av_frame_get_buffer() instead of ff_get_video_buffer()? Signed-off-by: Marton Balint <cus@passwd.hu> --- libavfilter/vf_zscale.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)