Adding png chunk cartridge support #2045

bztsrc · 2022-12-08T00:04:31Z

As explained in isse #2042 steganography has certain limitations. This patch add support storing the cartridge data in caRt PNG chunk as well. For backward compatibility, it still tries to use steganography if the cartridge fits into the pixels, and on load if no caRt chunk found, still tries steganography.

nesbox · 2022-12-09T10:45:47Z

src/ext/png.c

@@ -141,14 +165,25 @@ png_buffer png_write(png_img src)
        PNG_FILTER_TYPE_DEFAULT
    );

+    // Save cartridge data in a chunk too. This supports bigger cartridges than steganography
+    if (cart.data && cart.size > 0 && cart.size <= 0x7fffff){


Here you store the cart data in the caRt chunk in addition to the steganographic data and it looks redundant, we are doubling the cart data in two places. I suggest letting the user decide how to save the .png by adding console command options like save cart.png [way=(steno | caRt | both)].
What do you think?

(bztsrc is having issues logging into github and asked someone to forward this response.)

Here you store the cart data in the caRt chunk in addition to the steganographic data and it looks redundant, we are doubling the cart data in two places.

Only for now to provide backward compatibility.

I suggest letting the user decide

I think it would be much wiser to abandon steganography entirely instead. My reasons: - steganography has very limited storage capacity (98k tops) - it silently fails if the cartridge data happens to be bigger than that - it is extremely vulnerable to data corruption - has no means at all to detect data corruption On the other hand, chunk based storage - has unlimited storage capacity (up to 2^31) - can't silently fail on save, it always works - has a CRC to detect data corruption (handled by libpng automatically)

I agree. I think we need to maintain backwards compatibility - to continue to READ old steganography but if you make changes and save that cartridge it should be encoded as PNG chunk metadata. Giving the user 3 different choices is kind of the worst of all approaches.

bztsrc · 2022-12-13T03:09:43Z

Ah, that stupid verification code email finally arrived, I can answer again.

to continue to READ old steganography but if you make changes and save that cartridge it should be encoded as PNG chunk metadata.

Agree. I haven't removed the stegano writer part yet, because we have to wait until everybody starts using the new version, but I agree after a transition period stegano-write should go, leaving only stegano-read for backward compatibility.

bztsrc · 2022-12-14T01:13:32Z

@nesbox: Review needed please.

Plus here's a little addition to cart.c, which fixes issue #2048 It loads the cartridge data without using libpng (which is not linked with libtic80core.a), and this adds support for PNG cartridges in libretro (and probably on other systems as well). Needs this PNG chunk writer PR to be merged first of course, as it can only read the chunk.

diff --git a/src/cart.c b/src/cart.c
index 9be32c7..2d66fe1 100644
--- a/src/cart.c
+++ b/src/cart.c
@@ -85,6 +85,37 @@ void tic_cart_load(tic_cartridge* cart, const u8* buffer, s32 size)
     memset(cart, 0, sizeof(tic_cartridge));
     const u8* end = buffer + size;
 
+    // NOTE by bzt: we do not have libpng support here in libtic80core.a, so steganography is
+    // completely out of question, but we can find and uncompress a chunk ourself pretty easily
+    u8 *chunk_cart = NULL;
+
+    // check if this cartridge is in PNG format
+    if (!memcmp(buffer, "\x89PNG", 4))
+    {
+        s32 siz;
+        const u8* ptr = buffer + 8;
+        // iterate on chunks until we find a cartridge
+        while (ptr < end)
+        {
+            siz = ((ptr[0] << 24) | (ptr[1] << 16) | (ptr[2] << 8) | ptr[3]);
+            if (!memcmp(ptr + 4, "caRt", 4) && siz > 0)
+            {
+                chunk_cart = malloc(TIC_BINARY_SIZE);
+                if (chunk_cart)
+                {
+                    size = tic_tool_unzip(chunk_cart, TIC_BINARY_SIZE, ptr + 8, siz);
+                    buffer = chunk_cart;
+                    end = buffer + size;
+                }
+                break;
+            }
+            ptr += siz + 12;
+        }
+        // error, no TIC-80 cartridge chunk in PNG???
+        if (!chunk_cart)
+            return;
+    }
+
 #define LOAD_CHUNK(to) memcpy(&to, ptr, MIN(sizeof(to), chunk->size ? retro_le_to_cpu16(chunk->size) : TIC_BANK_SIZE))
 
     // load palette chunk first
@@ -219,6 +250,10 @@ void tic_cart_load(tic_cartridge* cart, const u8* buffer, s32 size)
                 }
         }
     }
+
+    // if we have allocated the buffer from a PNG chunk
+    if (chunk_cart)
+        free(chunk_cart);
 }

Cheers,
bzt

joshgoebel · 2022-12-14T02:56:17Z

I like that we get this advantage effectively "for free" if we switch to chunks.

bztsrc · 2022-12-27T09:27:47Z

Happy Xmas!

@nesbox: any chance of a review please?

Cheers,
bzt

nesbox · 2022-12-28T10:36:28Z

@bztsrc hi, sorry for the delay with the review, too little time due to moving to another country.
In my opinion we should handle both scenarios here (caRt + shorthand) or nothing at all, ideally it could be somewhere in one place and I don't see any problem to link libpng with tic80core.

bztsrc · 2022-12-31T09:25:42Z

@nesbox: no worries!

In my opinion we should handle both scenarios here (caRt + shorthand) or nothing at all

Not sure what you mean. This patch handles both, and with the cart.c patch it even handles both tic and png cartridges in libretro (a feature that's not working ATM).

ideally it could be somewhere in one place

Definitely, but that would require a refactor, out of scope of this PR which is about to introduce png chunk storage. Could be done a future PR, but first this functionality should be added.

I don't see any problem to link libpng with tic80core.

But I do. Unnecessary bloat for nothing. Implementing steganography in tic80core requires a lot of work for a technology that's obsoleted by this PR.

This PR isn't about hacking stegano into libtic80core, this is about the replace stegano with a much better solution (that as a side effect, solves the libtic80core png loading issue too). But the emphasis is on replacing the unreliable, vulnerable and limited capacity stegano in the first place.

Cheers,
bzt

joshgoebel · 2023-01-01T02:28:47Z

This PR isn't about hacking stegano into libtic80core, this is about the replace stegano with a much better solution (that as a side effect, solves the libtic80core png loading issue too).

Exactly, 100% agree. The new proposed solution paves the road for easily integrating PNG loading support in additional places, with almost (exactly?) zero new dependencies - that's a win win.

If someone else LATER wanted to come along and add steganography support to libtic80core - that would be a sep PR. But if this new method is better in all regards I also don't see the point in continuing to push steganography, esp. with the states known issues and incompatibilities.

Do you disagree with the new method is much better?

bztsrc · 2023-01-02T18:23:03Z

Happy new year!

with almost (exactly?) zero new dependencies

Exactly zero. The cart.c patch only needs tic_tool_unzip, which libtic80core.a already has, no additional libraries/functions required.

Do you disagree with the new method is much better?

Yeah, I'd like to hear your opinion too, if you don't want this PR, I wouldn't force it any more.

Cheers,
bzt

joshgoebel · 2023-01-02T20:44:39Z

@bztsrc Bigger picture would you be interested in doing any of the work to deprecate/remove steganography if we decided to go this direction? For example (one idea): removing the functionality completely - and replacing it with a tiny utility (or script) that converts a PNG from steganography to a DATA chunk. A webpage could possibly even do this...

I wish we know how commonly cartridges are even passed around as PNGs out there in the wild...

bztsrc · 2023-01-03T13:19:07Z

Bigger picture would you be interested in doing any of the work to deprecate/remove steganography if we decided to go this direction?

Considering that I've already done the bigger part in this PR, yes, sure. If this PR gets merged, I'll add a new PR that refactors png loading, removing stegano-write (I'd keep stegano-read for compatibility), and using one common function to decode png.

replacing it with a tiny utility (or script) that converts a PNG from steganography to a DATA chunk. A webpage could possibly even do this...

I've already created this tool, and it is also available as a web-service (via wasm) here, but you have my permission to add it to the TIC-80 webpage. Originally it was created to convert PICO-8 cartridges to TIC-80, but it can also convert .png files (either with stegano or chunk) to .tic and vice versa.

If the input is a .tic file, then the output is a .png which includes the data as stegano (if it fits, for backward compatibility) as well as in a chunk. This tool uses the default cartridge image and the default font, grabs the title and the author from the script's comments, so the resulting png is 100% pixel correct with the png files saved using TIC-80.

Cheers,
bzt

nesbox

maybe the problem is me, because I implemented the stenography, I don't want to refuse it and because of this I don't see the advantages of the new way :)
ok, let's merge the PR and see how it goes

joshgoebel · 2023-01-14T13:48:19Z

because of this I don't see the advantages of the new way

Isn't it pretty clear steno doesn't have enough bytes for large cartridges?

bztsrc added 4 commits December 8, 2022 00:50

Update xplode.c

2c3666a

Update console.c

a67c4d6

Update png.h

acd4e42

Update png.c

78fc5ce

nesbox reviewed Dec 9, 2022

View reviewed changes

bztsrc mentioned this pull request Dec 13, 2022

Unable to run PNG cartridges using libretro core #2048

Closed

nesbox approved these changes Jan 12, 2023

View reviewed changes

nesbox assigned bztsrc Jan 12, 2023

nesbox merged commit 1ce59dc into nesbox:main Jan 12, 2023

bztsrc mentioned this pull request Jan 13, 2023

Add png chunk loading #2088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding png chunk cartridge support #2045

Adding png chunk cartridge support #2045

bztsrc commented Dec 8, 2022

nesbox Dec 9, 2022

dulsi Dec 9, 2022 •

edited

Loading

joshgoebel Dec 10, 2022 •

edited

Loading

bztsrc commented Dec 13, 2022

bztsrc commented Dec 14, 2022

joshgoebel commented Dec 14, 2022

bztsrc commented Dec 27, 2022

nesbox commented Dec 28, 2022

bztsrc commented Dec 31, 2022

joshgoebel commented Jan 1, 2023

bztsrc commented Jan 2, 2023

joshgoebel commented Jan 2, 2023

bztsrc commented Jan 3, 2023

nesbox left a comment

joshgoebel commented Jan 14, 2023

Adding png chunk cartridge support #2045

Adding png chunk cartridge support #2045

Conversation

bztsrc commented Dec 8, 2022

nesbox Dec 9, 2022

Choose a reason for hiding this comment

dulsi Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

joshgoebel Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

bztsrc commented Dec 13, 2022

bztsrc commented Dec 14, 2022

joshgoebel commented Dec 14, 2022

bztsrc commented Dec 27, 2022

nesbox commented Dec 28, 2022

bztsrc commented Dec 31, 2022

joshgoebel commented Jan 1, 2023

bztsrc commented Jan 2, 2023

joshgoebel commented Jan 2, 2023

bztsrc commented Jan 3, 2023

nesbox left a comment

Choose a reason for hiding this comment

joshgoebel commented Jan 14, 2023

dulsi Dec 9, 2022 •

edited

Loading

joshgoebel Dec 10, 2022 •

edited

Loading