[FFmpeg-devel] [PATCH] mov: Skip computing SAR from invalid display matrix elements

[Anton Khirnov]

Quoting Vittorio Giovara (2021-04-01 12:09:36)
> On Thu, Apr 1, 2021 at 10:09 AM Anton Khirnov <anton at khirnov.net> wrote:
> 
> > Quoting Vittorio Giovara (2021-03-31 23:18:01)
> > > On Wed, Mar 31, 2021 at 8:54 PM Anton Khirnov <anton at khirnov.net> wrote:
> > >
> > > > Quoting Vittorio Giovara (2021-03-31 18:43:02)
> > > > > On Wed, Mar 31, 2021 at 2:41 PM Anton Khirnov <anton at khirnov.net>
> > wrote:
> > > > >
> > > > > > Quoting Vittorio Giovara (2021-03-30 18:55:27)
> > > > > > > Hello,
> > > > > > > I was debugging an issue with a video file containing an invalid
> > > > > > > display matrix, probably produced by a non conforming software.
> > > > > > >
> > > > > > > The content of the matrix is:
> > > > > > > 00000000:            0       65536           0
> > > > > > > 00000001:           -1           0           0
> > > > > > > 00000002:            0           0  1073741824
> > > > > > >
> > > > > > > The -1 (stored as 4294967295) was probably a 1 shifted 32 times
> > > > instead
> > > > > > > of 16. The problem is that this value is bypassing the validation
> > > > check
> > > > > > > in the code below, and the resulting computed SAR value becomes
> > > > 1:65536.
> > > > > > >
> > > > > > > This change interprets extremely low entries as invalid and makes
> > > > sure
> > > > > > > to skip them in the SAR computation. This passes fate, but I
> > haven't
> > > > been
> > > > > > > able to test this extensively.
> > > > > > > Please see the attached patch, any feedback or better solution is
> > > > > > welcome.
> > > > > > > --
> > > > > > > Vittorio
> > > > > > >
> > > > > > > From 54ec72276cbb6f2536e73ff81b7d49a736ec1900 Mon Sep 17 00:00:00
> > > > 2001
> > > > > > > From: Vittorio Giovara <vittorio.giovara at gmail.com>
> > > > > > > Date: Tue, 30 Mar 2021 16:47:39 +0200
> > > > > > > Subject: [PATCH] mov: Skip computing SAR from invalid display
> > matrix
> > > > > > elements
> > > > > > >
> > > > > > > ---
> > > > > >
> > > > > > I'm wondering if that code should set sample_aspect_ratio at all.
> > There
> > > > > > are two other bits of code in mov.c that may set
> > sample_aspect_ratio,
> > > > > > and it's not clear whether it applies _in addition_ to the display
> > > > > > matrix or not.
> > > > > >
> > > > >
> > > > > Unfortunately there are many (old-ish) samples that rely on the
> > display
> > > > > matrix to adjust the aspect ratio.
> > > > > We added a test for this case in particular, check out
> > fate-mov-zombie if
> > > > > you have some time.
> > > >
> > > > But then what do you do if a file has both the display matrix and the
> > > > pasp atom?
> > > >
> > >
> > > Right now the display matrix is parsed first, and pasp values are
> > ignored,
> > > perhaps we could change the behaviour so that pasp values take
> > precedence.
> > > If this is the preference I can send a separate patch about it.
> >
> > I would actually think that _both_ should be applied. But I am not an
> > expert on mp4, so maybe that's wrong. In any case, we should document
> > what our API expects or the users will be confused.
> >
> 
> Well in the sample attached to trac the second aspect ratio is 1:1 so if
> both were applied the end result would still be broken.
> I have the suspicion that the pasp atom is used as a stopgap to fix AR for
> software that does not generate a correct display matrix.
> 
> Where would this additional documentation need to be?

In the doxy for sample_aspect_ratio/DISPLAYMATRIX side data?

> 
> Going back on topic, any more feedback for the provided fix?

It's not pretty, but I won't block it.

-- 
Anton Khirnov