[FFmpeg-devel] [PATCH v4 1/3] avcodec/sanm: ignore unknown codecs in FOBJs

[FFmpeg-devel] [PATCH v4 1/3] avcodec/sanm: ignore unknown codecs in FOBJs

[Manuel Lauss]

Don't error out, just ignore unknown codec numbers and pretend
decode succeeded.  This is useful for older LucasArts titles
which stack a lot of different FOBJs with different codecs into
a single frame.

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
v4: no changes
v3: no changes
v2: Mark frame as corrupt, suggested by Marton Balint
 libavcodec/sanm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
index c30095ed32..a4f0a28c7c 100644
--- a/libavcodec/sanm.c
+++ b/libavcodec/sanm.c
@@ -1274,7 +1274,8 @@ static int process_frame_obj(SANMVideoContext *ctx)
         return old_codec48(ctx, w, h);
     default:
         avpriv_request_sample(ctx->avctx, "Subcodec %d", codec);
-        return AVERROR_PATCHWELCOME;
+        ctx->frame->flags |= AV_FRAME_FLAG_CORRUPT;
+        return 0;
     }
 }
 
-- 
2.48.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Manuel Lauss]

The left and top parameters of an FOBJ are signed values.

Signed-off-by: Manuel Lauss <manuel.lauss@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);
 
-- 
2.48.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v4 3/3] avcodec/sanm: add smush codec23 decoder

[Manuel Lauss]

This codec alternatingly skips and changes existing pixels.
A second 16bit parameter in the FOBJ header indicates how to do
the pixel changes: either by specifying a LUT in the codec datastream
or by adding a constant value to the pixel.
For ANIMv1 (Rebel Assault 1) the first 8bit parameter is used as
a constant offset to add to the existing pixel value.

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
---
v4: also support c23 on ANIMv0/1 (Rebel Assault 1, e.g LVL11/L11PLAY.ANM water)
v3: updates due to changes in patch 2
v2: no changes.

Videos showing the before/after state (Rebel Assault II LEV09/09PLAY.SAN file)
http://mlau.at/ffmpeg_c23_before.mp4
http://mlau.at/ffmpeg_c23_after.mp4
notice the blue transparent forcefields.

 libavcodec/sanm.c | 66 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
index 71dbac4320..534ec7aee6 100644
--- a/libavcodec/sanm.c
+++ b/libavcodec/sanm.c
@@ -292,6 +292,7 @@ typedef struct SANMVideoContext {
     int8_t p4x4glyphs[NGLYPHS][16];
     int8_t p8x8glyphs[NGLYPHS][64];
     uint8_t c47itbl[0x10000];
+    uint8_t c23lut[256];
 } SANMVideoContext;
 
 typedef struct SANMFrameHeader {
@@ -555,6 +556,62 @@ static int rle_decode(SANMVideoContext *ctx, uint8_t *dst, const int out_size)
     return 0;
 }
 
+static int old_codec23(SANMVideoContext *ctx, int top, int left, int width,
+                       int height, uint8_t param, uint16_t param2)
+{
+    const uint32_t maxpxo = ctx->width * ctx->pitch;
+    uint8_t *dst, lut[256], c;
+    int i, j, k, pc, sk;
+    int32_t pxoff;
+
+    if (ctx->subversion < 2) {
+        /* Rebel Assault 1: constant offset + 0xd0 */
+        for (i = 0; i < 256; i++)
+            lut[i] = (i + param + 0xd0) & 0xff;
+    } else if (param2 == 256) {
+        if (bytestream2_get_bytes_left(&ctx->gb) < 256)
+            return AVERROR_INVALIDDATA;
+        bytestream2_get_bufferu(&ctx->gb, ctx->c23lut, 256);
+    } else if (param2 < 256) {
+        for (i = 0; i < 256; i++)
+            lut[i] = (i + param2) & 0xff;
+    } else {
+        memcpy(lut, ctx->c23lut, 256);
+    }
+    if (bytestream2_get_bytes_left(&ctx->gb) < 1)
+        return 0;  /* some c23 frames just set up the LUT */
+
+    dst = (uint8_t *)ctx->frm0;
+    for (i = 0; i < height; i++) {
+        if (bytestream2_get_bytes_left(&ctx->gb) < 2)
+            return 0;
+        pxoff = left + ((top + i) * ctx->pitch);
+        k = bytestream2_get_le16u(&ctx->gb);
+        sk = 1;
+        pc = 0;
+        while (k > 0 && pc <= width) {
+            if (bytestream2_get_bytes_left(&ctx->gb) < 1)
+                return AVERROR_INVALIDDATA;
+            j = bytestream2_get_byteu(&ctx->gb);
+            if (sk) {
+                pxoff += j;
+                pc += j;
+            } else {
+                while (j--) {
+                    if (pxoff >=0 && pxoff < maxpxo) {
+                        c = *(dst + pxoff);
+                        *(dst + pxoff) = lut[c];
+                    }
+                    pxoff++;
+                    pc++;
+                }
+            }
+            sk ^= 1;
+        }
+    }
+    return 0;
+}
+
 static int old_codec1(SANMVideoContext *ctx, int top,
                       int left, int width, int height)
 {
@@ -1237,11 +1294,15 @@ 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 parm2;
+    uint8_t  codec = bytestream2_get_byteu(&ctx->gb);
+    uint8_t  param = bytestream2_get_byteu(&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);
+    bytestream2_skip(&ctx->gb, 2);
+    parm2 = bytestream2_get_le16u(&ctx->gb);
 
     if (!w || !h) {
         av_log(ctx->avctx, AV_LOG_ERROR, "Dimensions are invalid.\n");
@@ -1260,12 +1321,13 @@ static int process_frame_obj(SANMVideoContext *ctx)
             return AVERROR(ENOMEM);
         }
     }
-    bytestream2_skip(&ctx->gb, 4);
 
     switch (codec) {
     case 1:
     case 3:
         return old_codec1(ctx, top, left, w, h);
+    case 23:
+        return old_codec23(ctx, top, left, w, h, param, parm2);
     case 37:
         return old_codec37(ctx, top, left, w, h);
     case 47:
-- 
2.48.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1424 bytes --]

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@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 ?

thx

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 1/3] avcodec/sanm: ignore unknown codecs in FOBJs

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 718 bytes --]

Hi Manuel

On Tue, Mar 04, 2025 at 06:07:17PM +0100, Manuel Lauss wrote:
> Don't error out, just ignore unknown codec numbers and pretend
> decode succeeded.  This is useful for older LucasArts titles
> which stack a lot of different FOBJs with different codecs into
> a single frame.
> 
> Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
> ---
> v4: no changes
> v3: no changes
> v2: Mark frame as corrupt, suggested by Marton Balint
>  libavcodec/sanm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

will apply

thx

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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@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@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2440 bytes --]

Hi

On Sun, Mar 09, 2025 at 04:52:25PM +0100, Manuel Lauss wrote:
> Hi Michael,
> 
> On Sat, Mar 8, 2025 at 8:11 PM Michael Niedermayer
> <michael@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@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).

My concern is not that it crashes my concern is that manually craftet
files could result in arbitrary code execution if theres any out of
array accesses.
Did you check that negative values are safe in that respect ?

thx

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

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".