On Thu, Apr 1, 2021 at 10:09 AM Anton Khirnov <an...@khirnov.net> wrote:
> Quoting Vittorio Giovara (2021-03-31 23:18:01) > > On Wed, Mar 31, 2021 at 8:54 PM Anton Khirnov <an...@khirnov.net> wrote: > > > > > Quoting Vittorio Giovara (2021-03-31 18:43:02) > > > > On Wed, Mar 31, 2021 at 2:41 PM Anton Khirnov <an...@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.giov...@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? Going back on topic, any more feedback for the provided fix? -- Vittorio _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
