[FFmpeg-devel] [PATCH v4 2/3] avcodec/sanm: fobj left/top are signed

[Manuel Lauss]

Hi Michael,

On Sat, Mar 8, 2025 at 8:11 PM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> Hi Manuel
>
> On Tue, Mar 04, 2025 at 06:07:18PM +0100, Manuel Lauss wrote:
> > The left and top parameters of an FOBJ are signed values.
> >
> > Signed-off-by: Manuel Lauss <manuel.lauss at gmail.com>
> > ---
> > v4: revert v3, it arose due to a misunderstanding
> > v3: change the bytestream accessor to signed too
> > v2: no changes
> >  libavcodec/sanm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
> > index a4f0a28c7c..71dbac4320 100644
> > --- a/libavcodec/sanm.c
> > +++ b/libavcodec/sanm.c
> > @@ -1238,8 +1238,8 @@ static int old_codec48(SANMVideoContext *ctx, int width, int height)
> >  static int process_frame_obj(SANMVideoContext *ctx)
> >  {
> >      uint16_t codec = bytestream2_get_le16u(&ctx->gb);
> > -    uint16_t left  = bytestream2_get_le16u(&ctx->gb);
> > -    uint16_t top   = bytestream2_get_le16u(&ctx->gb);
> > +    int16_t  left  = bytestream2_get_le16u(&ctx->gb);
> > +    int16_t  top   = bytestream2_get_le16u(&ctx->gb);
> >      uint16_t w     = bytestream2_get_le16u(&ctx->gb);
> >      uint16_t h     = bytestream2_get_le16u(&ctx->gb);
>
> Does the following code also handle all error conditions that
> negative left/top could now trigger ?

For the LucasArts titles that sanm.c currently supports well,
no negative values are ever encountered.
I let ffplay run through maybe 1/3 of the Rebel Assault 1 videos,
which are the only ones that make use of negative values, but
didn't encounter any crashes; mostly because the codecs it
uses aren't supported by ffmpeg/sanm (yet).

Manuel