-
-
Notifications
You must be signed in to change notification settings - Fork 758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Add projector filter upstream from SWF creation (Close: #11539) #15788
base: master
Are you sure you want to change the base?
Core: Add projector filter upstream from SWF creation (Close: #11539) #15788
Conversation
d687d47
to
e6aa6de
Compare
core/src/tag_utils.rs
Outdated
@@ -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], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit: This name might no longer be the best fit. Nominating renaming it to just data
.
core/src/tag_utils.rs
Outdated
} | ||
|
||
/// Get an SWF from data if the data is a swf/projector bundle | ||
fn filter_from_projector_bundle(data: &[u8]) -> Option<Vec<u8>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't return a slice itself without boxing, but you can return the borrow to it.
core/src/tag_utils.rs
Outdated
let signature = u32::from_le_bytes( | ||
data[len - 8..len - 4] | ||
.try_into() | ||
.expect("4 bytes make a u32"), | ||
); | ||
|
||
if signature != 0xFA123456 { | ||
return None; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core/src/tag_utils.rs
Outdated
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.)
core/src/tag_utils.rs
Outdated
for i in 0..swf_size { | ||
swf_data.push(data[i + offset]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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. 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. |
Also I think maybe it's worth considering adding |
Hmm, we probably shouldn't be doing this logic in |
Good point about moving it into 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 |
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). |
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
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". |
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 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
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. |
@@ -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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call this decompress_...
, as there is only a bit of slicing done.
Maybe extract_swf_from_...
- but something shorter? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Sooo... should this be in |
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. |
For issue: #11539
Allows ruffle to filter and play SWFs from standalone projector exe bundles.
Tested With: