[FFmpeg-devel] [PATCH 08/10] swscale/swscale_unscaled: Fix odd height with nv24_to_yuv420p_chroma()

[Michael Niedermayer]

On Wed, Sep 25, 2024 at 03:16:30PM +0200, Ramiro Polla wrote:
> On Tue, Sep 24, 2024 at 3:35 PM Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> > On Mon, Sep 23, 2024 at 12:42:22AM +0200, Ramiro Polla wrote:
> > > Hi,
> > >
> > > On Mon, Sep 23, 2024 at 12:04 AM Michael Niedermayer
> > > <michael at niedermayer.cc> wrote:
> > > >
> > > > Fixes: out of array read
> > > > Fixes: 71726/clusterfuzz-testcase-ffmpeg_SWS_fuzzer-5876893532880896
> > > >
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > >  libswscale/swscale_unscaled.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
> > > > index dc1d5f35932..d403c953cc7 100644
> > > > --- a/libswscale/swscale_unscaled.c
> > > > +++ b/libswscale/swscale_unscaled.c
> > > > @@ -230,6 +230,8 @@ static void nv24_to_yuv420p_chroma(uint8_t *dst1, int dstStride1,
> > > >      const uint8_t *src2 = src + srcStride;
> > > >      // average 4 pixels into 1 (interleaved U and V)
> > > >      for (int y = 0; y < h; y += 2) {
> > > > +        if (y + 1 == h)
> > > > +            src2 = src1;
> > > >          for (int x = 0; x < w; x++) {
> > > >              dst1[x] = (src1[4 * x + 0] + src1[4 * x + 2] +
> > > >                         src2[4 * x + 0] + src2[4 * x + 2]) >> 2;
> > >
> > > I would prefer to keep nv24_to_yuv420p_chroma() expecting height to be
> > > a multiple of 2. We could add && !(c->srcH & 1) before selecting
> > > nv24ToYuv420Wrapper.
> >
> > what advantage does this have ?
> 
> This would also have the benefit of keeping the function clearer, and
> we wouldn't have to add special cases that might or might not be ok.

> It would also make it easier to write simd code, since we wouldn't
> have to worry about these special cases.

The SIMD code could support a subset of the c code


> 
> It is also easy to forget these corner cases, which leads to some
> converters behaving differently in corner cases. For example, the
> current yuyv422 to yuv420p scaler just doesn't output the last line if
> it has an odd height. What is the correct fix? Should we convert the
> last line differently like in this case? Should we duplicate the
> second-to-last line? Should we just add a couple lines of code to
> tweak the converter? Should we instead call this function with an even
> height, and call another function for the last line?

>
> IMO a converter to yuv420p with odd width or height shouldn't be an
> unscaled converter, since we're effectively scaling the chroma planes
> (and not just by a factor of two). In this case, the regular scaler
> would be used.

Iam not sure i understand correctly, but
assuming a 99 lines 4:4:4 scaled to 99 lines 4:2:0
the chroma here is not a simple "99 to 50 lines rescaling"
the chroma samples must stay co-located in relation to the luma samples
that can only happen with s 2:1 not a 99:50 rescaling
but maybe i misunderstood what you meant

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240928/d34180e6/attachment.sig>