Core: Add projector filter upstream from SWF creation (Close: #11539) #15788

TheDecimeter · 2024-03-27T01:30:19Z

For issue: #11539

Allows ruffle to filter and play SWFs from standalone projector exe bundles.

Tested With:

"Clippy" projectors from Internet Archive (as files and urls)
Custom projectors created with Node package: https://github.com/shockpkg/swf-projector

torokati44 · 2024-03-28T11:08:47Z

core/src/tag_utils.rs

@@ -151,11 +151,59 @@ impl SwfMovie {
        Self::from_data(&data, url.into(), loader_url)
    }

-    /// Construct a movie based on the contents of the SWF datastream.
+    /// Construct a movie based on the contents of a datastream.
    pub fn from_data(
        swf_data: &[u8],


Just a nit: This name might no longer be the best fit. Nominating renaming it to just data.

torokati44 · 2024-03-28T11:13:56Z

core/src/tag_utils.rs

+    }
+
+    /// Get an SWF from data if the data is a swf/projector bundle
+    fn filter_from_projector_bundle(data: &[u8]) -> Option<Vec<u8>> {


Not important, just a little note: Do you think it would be possible to make this return an Option<&[u8]> instead of an Option<Vec<u8>>? Just to avoid a copy. Doing this might involve a lifetime annotation or two. I don't have a definite solution in my head, maybe it's not feasible.
It's also fine if it stays as is though.

You know... I think it is (haven't had time to test fully, but a little playing around seemed promising).

I probably spent a week trying to figure out how to return a slice but kept failing. Adding the Option/Some seems to make it happy, I'll have to look more into that (pretty new to Rust).

You can't return a slice itself without boxing, but you can return the borrow to it.

torokati44 · 2024-03-28T11:22:38Z

core/src/tag_utils.rs

+        let signature = u32::from_le_bytes(
+            data[len - 8..len - 4]
+                .try_into()
+                .expect("4 bytes make a u32"),
+        );
+
+        if signature != 0xFA123456 {
+            return None;
+        }


Suggestion, just for simpler code: data[len - 8..len - 4] != [0xFA, 0x12, 0x34, 0x56]. No need to convert to an u32 necessarily.
Again, this is just a minor nit, not a blocker for merging.

Yep, I originally did it this way because I liked the "readable" way the u32 flowed "..123456". The array would have that reversed. But you're right that it'd be a smaller line of code so that's a fair point.

torokati44 · 2024-03-28T11:31:37Z

core/src/tag_utils.rs

+        let swf_size = usize::try_from(u32::from_le_bytes(
+            data[len - 4..len].try_into().expect("4 bytes make a u32"),
+        ))
+        .expect("size should be at least 32 bits");


Shouldn't this also work?

let swf_size = data[len - 4..].try_into().expect("4 bytes make a u32") as usize;

An u32 -> usize cast feels fairly harmless. (Also the end of the range is redundant if it's len.)

torokati44 · 2024-03-28T11:37:13Z

core/src/tag_utils.rs

+        for i in 0..swf_size {
+            swf_data.push(data[i + offset]);
+        }


Vec::extend_from_slice might be more efficient, at the very least clearer.
Anyway, if the return type is changed to Option<&[u8]>, this won't be necessary at all.

Yep, wasn't aware of that function so thank you for pointing it out (but yeah, looks like the &[u8] idea will probably work fine)

torokati44 · 2024-03-28T11:40:07Z

Welcome, and thank you! ^^

I've posted a bunch of little notes - but know that I'm known to be really nitpicky, as also mentioned in the comments.

If you feel like addressing them, that would be great - but overall your changes look fine already.
Especially since it adds a useful feature, with a relatively small code change footprint.

Oh and maybe a link or two in the code itself to a couple of the half dozen projects/repos/blogposts/tools that document this format would be nice.

torokati44 · 2024-03-28T14:04:12Z

Also I think maybe it's worth considering adding .exe as an option for file type filter in actually_pick_file in desktop.

Lord-McSweeney · 2024-03-28T18:45:26Z

Hmm, we probably shouldn't be doing this logic in tag_utils, since that's meant for processing regular SWFs. IMO this would be better as a desktop option (something like --filter-bundled-swf).

torokati44 · 2024-03-28T19:40:41Z

Good point about moving it into desktop. I'm not convinced an option is needed for this - but if so, I think it should be enabled by default ... and if so, why or when woukd one want to disable it? 🤔

Oh, one thing: I think this should be tried as a fallback, if an SWF header wasn't found. Because with how lax SWF is, one could easily look like a projector binary if only it's very end is checked.

One more thing: I think this should never panic, only return None if it couldn't extract an SWF.

TheDecimeter · 2024-03-29T05:32:37Z

Hello, thanks for all the feedback.

Tag-utils was chosen since that was the spot where it seemed like all the data flowed to (if I go up stream from that there seem to be many data entry points to cover). It seemed like this was where SWFs were being parsed, so it felt like a logical part to put a filter in that strips out the garbage that might be put around the SWF file we're hoping for.

Desktop-only entry points were avoided since exes could also be fed in via browser extension (ruffle-android is still early I think, so this could be useful for Kiwi browser users).

I am curious what you think about the above two points (but these types of files are rare, so I doubt much is lost in keeping the feature out of browser extensions if that is where we settle)

Good thought to look into headers (or just check the file extension for .exe) if there's a concern about SWFs inadvertently triggering this (I'll see what my options are there).

Dinnerbone · 2024-04-01T20:32:48Z

Also I think maybe it's worth considering adding .exe as an option for file type filter in actually_pick_file in desktop.

Agree with this as an additional filter to select from - but the default filter should not include exes as it's quite unlikely that a given exe is playable

Hmm, we probably shouldn't be doing this logic in tag_utils, since that's meant for processing regular SWFs. IMO this would be better as a desktop option (something like --filter-bundled-swf).

I don't think it should be an option (if you tried to open an exe instead of a swf, you probably meant to play the exe as a swf) - but I do strongly agree that this isn't the right place for it.

Desktop should be trying to find the swf itself and passing it over to the ruffle player, rather than asking the ruffle player to figure out what to do here. If we do indeed want this as a common thing elsewhere (opening exe on android... I guess it makes sense but also kinda funky) then it should be some utility method that a frontend can go through, but it's still "I'm finding the swf from the user input and giving it to ruffle", rather than "I'm giving ruffle an unknown byte stream and telling it to figure it out".

torokati44 · 2024-04-01T23:55:05Z

If we do indeed want this as a common thing elsewhere (opening exe on android... I guess it makes sense but also kinda funky) then it should be some utility method that a frontend can go through, but it's still "I'm finding the swf from the user input and giving it to ruffle", rather than "I'm giving ruffle an unknown byte stream and telling it to figure it out".

While trying out the "Ruffle Projector" feature based on this, I also arrived at the same conclusion.

So, I think, there should be some sort of utility function, which can take in both simple SWFs and projector bundles, then return just the SWF part from within it (which is the whole thing in the former case).

The issue with this is that currently, AFAIU, desktop only passes an URL to Player, and the loading/fetching is handled in core. For this to work, we'll need to add a method to directly pass in an SWF data slice to play from the frontends.

This standalone extractor function will also be more straightforward to extend later to return the non-projector EXE instead (for creating a different Ruffle projector from a Ruffle projector), or to understand a different magic number for "game-asset bundle" extraction once those are a thing...

Filter now occurs only if SWF fails to parse and re-calls the parse method

TheDecimeter · 2024-04-02T03:06:21Z

well... the latest commit doesn't address anything from this evening's comments (sorry, didn't notice them till just now).

Currently, this just applied suggestions from torokati44 and restructured the code to be stored in read.rs (but it is still called from the same load_data method).

It is now a fallback for when traditional SWF parsing fails (as suggested).

If it is decided that we don't want this as a fallback (to instead try filtering first, then pass to SWF data to the player from the various entry points), I can look into that.

torokati44 · 2024-04-02T09:18:13Z

swf/src/read.rs

@@ -52,6 +52,33 @@ pub fn extract_swz(input: &[u8]) -> Result<Vec<u8>> {
    Err(Error::invalid_data("Invalid ASN1 blob"))
 }

+/// Parses an SWF from data in an SWF projector bundle
+/// Returns an `Error` if this is not a valid SWF projector bundle.
+pub fn decompress_projector_bundle(data: &[u8]) -> Result<SwfBuf> {


I wouldn't call this decompress_..., as there is only a bit of slicing done.
Maybe extract_swf_from_... - but something shorter? :P

Oh the return type changed. The latest commit gives the same return value as "decompress_swf".

The idea was since this is a fallback, you first try decompress_swf, and if that fails, you try to get data via decompress_projector_bundle

torokati44 · 2024-04-09T09:38:12Z

Sooo... should this be in frontend-utils now? 🤔

TheDecimeter · 2024-04-19T03:21:13Z

Sorry, was out of town for the Eclipse

I took a quick look at the new frontend-utils folder didn't look promising (unless I'm missing something?). It doesn't seem like any of the necessary code paths go through there. I know it talks about "Bundles" and attempts to open one in the desktop/player.rs, but those seem to be different from the projector "exe bundle" mentioned here.

Unless the thought is to make an exebundle.rs file and move the function there and call it from elsewhere.

theDecimeter added 2 commits March 27, 2024 07:30

Add projector filter upstream from swf creation

204b555

add check to ensure there is enough data to look into

e6aa6de

torokati44 force-pushed the unbundle-projector-swfs branch from d687d47 to e6aa6de Compare March 27, 2024 06:30

torokati44 reviewed Mar 28, 2024

View reviewed changes

Merge branch 'master' into unbundle-projector-swfs

a3d3959

Simplified filter function and moved to read.rs

6a3d401

Filter now occurs only if SWF fails to parse and re-calls the parse method

torokati44 reviewed Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add projector filter upstream from SWF creation (Close: #11539) #15788

Core: Add projector filter upstream from SWF creation (Close: #11539) #15788

TheDecimeter commented Mar 27, 2024 •

edited

torokati44 Mar 28, 2024

torokati44 Mar 28, 2024

TheDecimeter Mar 29, 2024

lvyitian Apr 2, 2024

torokati44 Mar 28, 2024

TheDecimeter Mar 29, 2024

torokati44 Mar 28, 2024

torokati44 Mar 28, 2024

TheDecimeter Mar 29, 2024

torokati44 commented Mar 28, 2024 •

edited

torokati44 commented Mar 28, 2024

Lord-McSweeney commented Mar 28, 2024

torokati44 commented Mar 28, 2024

TheDecimeter commented Mar 29, 2024

Dinnerbone commented Apr 1, 2024

torokati44 commented Apr 1, 2024

TheDecimeter commented Apr 2, 2024

torokati44 Apr 2, 2024

TheDecimeter Apr 2, 2024

torokati44 commented Apr 9, 2024

TheDecimeter commented Apr 19, 2024

Core: Add projector filter upstream from SWF creation (Close: #11539) #15788

Are you sure you want to change the base?

Core: Add projector filter upstream from SWF creation (Close: #11539) #15788

Conversation

TheDecimeter commented Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torokati44 commented Mar 28, 2024 • edited

torokati44 commented Mar 28, 2024

Lord-McSweeney commented Mar 28, 2024

torokati44 commented Mar 28, 2024

TheDecimeter commented Mar 29, 2024

Dinnerbone commented Apr 1, 2024

torokati44 commented Apr 1, 2024

TheDecimeter commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torokati44 commented Apr 9, 2024

TheDecimeter commented Apr 19, 2024

TheDecimeter commented Mar 27, 2024 •

edited

torokati44 commented Mar 28, 2024 •

edited