[FFmpeg-devel] [FFmpeg-cvslog] avcodec/asvenc: Don't waste bits encoding non-visible part

[Andreas Rheinhardt]

Martin Storsjö:
> On Mon, 26 May 2025, Andreas Rheinhardt wrote:
> 
>> ffmpeg | branch: master | Andreas Rheinhardt
>> <andreas.rheinhardt at outlook.com> | Thu May 22 15:57:13 2025 +0200|
>> [0401ca714a2714743573e27c384ffa810fd31a92] | committer: Andreas
>> Rheinhardt
>>
>> avcodec/asvenc: Don't waste bits encoding non-visible part
>>
>> Up until now, the encoder replicated all the border pixels
>> for incomplete 16x16 macroblocks. In case the available width
>> or height is <= 8, some of the luma blocks of the MB
>> do not correspond to actual input, so that we should encode
>> them using the least amount of bits. Zeroing the block coefficients
>> (as this commit does) achieves this, replicating the pixels
>> and performing an FDCT does not.
>>
>> This commit also removes the frame copying code for insufficiently
>> aligned dimensions.
>>
>> The vsynth3-asv[12] FATE tests use a 34x34 input file and are
>> therefore affected by this. As the ref updates show, the size
>> and checksum of the encoded changes, yet the decoded output
>> stays the same.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>>
>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?
>>> a=commit;h=0401ca714a2714743573e27c384ffa810fd31a92
>> ---
>>
>> libavcodec/asvenc.c           | 131 +++++++++++++++++++++++++
>> +----------------
>> tests/ref/vsynth/vsynth3-asv1 |   4 +-
>> tests/ref/vsynth/vsynth3-asv2 |   4 +-
>> 3 files changed, 84 insertions(+), 55 deletions(-)
> 
> As noted on irc, this broke the fate-vsynth3-asv1 and fate-vsynth3-asv2
> tests on arm.
> 
> The issue is not AV_COPY128 as Andreas wondered on irc; the reason is a-
>>pdsp.get_pixels. We have a backtrace like this:
> 
> #0  0x00cfdd2a in ff_get_pixels_neon ()
>     at src/libavcodec/arm/pixblockdsp_neon.S:47
> #1  0x0082ea54 in dct_get (mb_y=0, mb_x=0, frame=0xf6202f40, a=0x297f450)
>     at src/libavcodec/asvenc.c:222
> #2  encode_frame (avctx=0x297f150, pkt=0xf6202ec0, pict=0xf6202f40,
>     got_packet=0xf7df4af0) at src/libavcodec/asvenc.c:304
> 
> The arguments to a->pdsp.get_pixels are this on entry:
> 
> (gdb) info registers r0 r1 r2
> r0             0x297f4a0           43512992
> r1             0x297caa0           43502240
> r2             0x22                34
> 
> After one iteration, it hits the bus error due to unaligned reads from
> the source, which was expected to stay aligned after incrementing by the
> stride.
> 

This means that the stride is unaligned. Which as I remember is an API
violation on the user's (FFmpeg CLI) side. The alignment requirements
for the frames passed to encoders are not really properly documented (in
contrast to the requirements for decoders). The best I could find is the
documentation of linesize in lavu/frame.h:
"For video the linesizes should be multiples of the CPUs alignment
preference, this is 16 or 32 for modern desktop CPUs.
Some code requires such alignment other code can be slower without
correct alignment, for yet other it makes no difference."
I could not find an explicit requirement that the data pointer need to
be sufficiently aligned.

It is possible to hit this bug even before my patch with a commandline
like this (tested with qemu):
ffmpeg -filter_complex
nullsrc=s=740x576:r=25,format=yuv420p,crop=w=720:x=2 -c:v asv2 -f null -
The same input also gives a bus error with dvvideo. dnxhd is similarly
affected by this (yet not the mpegvideo family, as it already checks the
alignment and makes a copy if necessary (it needs the frames to be
padded on all sides, so it is common for it to copy even properly
aligned input).

So do we consider this a bug in the asv encoders (and in dvvideo and
dnxhd)? Or is it an API violation on part of the caller? In the former
case, I could fix this by checking on a per-frame basis whether the
frame is properly aligned and switching between get_pixels_unaligned and
get_pixels on that basis.

- Andreas