core: Correct and implement MovieClip states (default state, initial loading state, error state, image state and unloaded state) #10792
Conversation
|
Thank you! Would it be possible for you to add a test for this? |
|
Is this also the correct behavior for AVM2? |
|
Thanks for the quick response; I'll try writing some tests and testing it in AVM2 as well :) |
|
@Lord-McSweeney Okay, I looked a bit into it and there is no possibility to access MovieClip variables in ActionScript 3 if the clip couldn't be loaded. |
|
@Dinnerbone I've spent some time writing detailed tests which test the default variables and the changes of the MovieClip state (this made me notice that not only _framesloaded but also other variables change in this situation). However, when trying to integrate them in Ruffle, I'm currently facing the problem that the _url property is also tested. When I execute that test, it includes my project root ( |
|
I'd say to split the URL to only retain the common parts. This is what I did in a past PR: var url = unescape(mc._url).split("/");
url = url[url.length - 2] + "/" + url[url.length - 1];
trace("mc._url (unescaped & split): " + url);Traces:
|
|
Thank you; that's a really good solution! |
|
Do tests not have access to the internet? I am still working on my test, but if I try to access an online SWF with |
|
I've converted this to a draft because I've noticed some other deviations to the Flash player during my tests; I'll reopen this when I've finished the tests and implemented the changes in Ruffle. |
Lord-McSweeney
added
the
waiting-on-author
Korne127
force-pushed
the
loader
branch
2 times, most recently
from
7d87130
to
f590eba
Compare
Korne127
force-pushed
the
loader
branch
2 times, most recently
from
95e1491
to
2b36a22
Compare
Korne127
changed the title
Korne127
changed the title
|
I finally finished implementing all the MovieClip states. Since the scope of this pull request has increased so much, this is a new summary of all of its changes: General concept:A MovieClip can be in different specific states. The state of a MovieClip consists of the values of all properties and the results of some getter functions of the MovieClip. Previously, the states have not been correctly implemented, which can lead to flash content not properly working (e.g. Paraplüsch / Paraplush didn't recognise the error state because Ruffle didn't implement it correctly; therefore it didn't work). New MovieClip states that have been implemented:
MovieClip states that have been corrected:
Added tests:
Other big changes:
Minor changes:
To implement this all, various functions, variables and structs have been created and changed. The individual commit messages contain more detailed information about that. |
|
Currently, there are just two minor problems. These are also the reason why some of the tests fail: However, Additionally to that, the |
|
@Aaron1011 @Lord-McSweeney Do you have other comments about this pull request or my answers? If not; I'd add a commit adapting it to the reviews. |
n0samu
added
waiting-on-review
| target.avm1_unload(&mut activation.context); | ||
| // TODO: Find out what's the correct behaviour. If target isn't a MovieClip, | ||
| // does Flash also wait a frame to execute avm1_unload? Is avm1_unload_movie | ||
| // the correct call? |
| reason: &str, | ||
| url: &str, | ||
| error: ErrorType, | ||
| ) -> Result<SuccessResponse, ErrorResponse> { |
There was a problem hiding this comment.
This is fine for now, but I suspect we will eventually need a more in-depth FetchError type than just "here's an error string". Perhaps that could also encompass ErrorResponse's URL parameter...
| ) { | ||
| // If a local URL is fetched using the flash plugin, the _url property | ||
| // won't be changed => It keeps being the parent SWF URL. | ||
| if cfg!(target_family = "wasm") { |
There was a problem hiding this comment.
I'm not entirely sure checking the compile architecture is correct to determine if we're in the web build. I mean, it works, but there's no particular reason why WASM needs to run in a browser. There's all sorts of non-browser WASM hosts now... though I'm not entirely sure if any of them would be appropriate places to embed Ruffle into.
At the very least this seems like something NavigatorBackend should be handling somehow.
There was a problem hiding this comment.
How would you suggest to do that? I could imagine a NavigatorBackend method like is_on_web which returns true in the WebNavigatorBackend and false in the others. If the wasm check isn't clean to test whether we're in a browser, this might be a better idea (although a cfg like this is used in two other places in Ruffle; it might make sense to replace them as well).
The test itself should be done in this method; otherwise it would need to be implemented in different places for both the initial loading and the error state since this applies to both and the method is called for both (if the file is local).
There was a problem hiding this comment.
Checking the compile architecture to determine the build type is something that we do in many places.
There was a problem hiding this comment.
We might want to have an explicit feature for at least some of the things that currently rely on target_family='wasm'. Again, entirely out of scope. For this particular PR alone I was more confused as to why NavigatorBackend's fancy new parse_url method wasn't being used to change file:// URLs.
There was a problem hiding this comment.
The new resolve_url method is just used to actually resolve the url string and do the necessary preprocessing, which depends on the NavigatorBackend implementation. It is used by both the navigate_to_url (which opens a URL link) and the fetch (which loads a movie) methods.
These methods need to know the correct resolved URL. E.g. if navigate_to_url is called with a local URL string, it needs to know that it's local, throws an error and doesn't open it.
What this code does, is setting the SwfMovie::url variable. And if ActionScript code tries to load a local file on web, Flash Player makes the MovieClip go into the error state but doesn't change the url, FP "lies about it" and it just stays the same.
But this just works if resolve_url returns the correct Url and this condition is specifically tested afterwards. If resolve_url itself would return this in the situation, fetch and navigate_to_url would think it's a valid url (the url of the parent SWF itself) and load / navigate there. So yeah, they need to know it's a local file / invalid and resolve_url needs to actually just resolve the URL correctly, but because FP lies in the error state, when setting this state, this needs to be deliberately tested and taken into account to set the state as FP does.
There was a problem hiding this comment.
The fetch API doesn't allow access to file URLs, so all of this is really inconsequential/pointless, right?
There was a problem hiding this comment.
@n0samu What exactly do you mean? The code highlighted in this review sets the url property of the MovieClip, which is accessible in ActionScript. If this isn't done, the url property of a MovieClip is wrong under these specific circumstances.
In general, the NavigationBackend refactor allows for better code quality and improves the error messages. Previously, you could also not load local files on web, but a generic "Got JS error" got returned, while now that case is directly handled when resolving the URL and a precise error message is returned.
There was a problem hiding this comment.
Oh okay, thanks for the explanation, don't mind me
There was a problem hiding this comment.
Dw; it's a good question
And you're welcome :)
| @@ -112,8 +112,19 @@ impl HeaderExt { | |||
| } | |||
| } | |||
|
|
|||
| /// Returns the header for the error state movie stub which is used if no file | |||
| /// could be loaded or if the loaded content is no valid supported content. | |||
| pub fn default_error_header() -> Self { | |||
There was a problem hiding this comment.
Why is this in swf instead of ruffle_core? swf is just a parsing library, and there's no inherent "error SWF" in the SWF spec.
There was a problem hiding this comment.
I thought it would be cleaner to have the different default headers that Ruffle uses for an SWF in different states all in the same place. Especially since both the success header (which is used in the success state) and the image header (which is used in the image state) are located here, I think the same makes sense for the error header (used in the error state).
There was a problem hiding this comment.
Aah, in that case we'll have to keep with what's already there. Maybe a future PR could move that out of swf.
| @@ -97,7 +97,7 @@ pub struct HeaderExt { | |||
| pub(crate) header: Header, | |||
| pub(crate) file_attributes: FileAttributes, | |||
| pub(crate) background_color: Option<SetBackgroundColor>, | |||
| pub(crate) uncompressed_len: u32, | |||
| pub(crate) uncompressed_len: i32, | |||
There was a problem hiding this comment.
I feel a little weird letting negative lengths leak into the SWF parsing machinery. Did Flash have a hard 2GB cap on SWF size (even on 64-bit)?
There was a problem hiding this comment.
I understand that. Sadly we can't just use an unsigned integer here because -1 is a valid value needed in some situations (and 0 is as well, so we can't just read 0 as -1).
One alternative to this I easily see would be an Option<u32> (with None standing for -1). However, this would make comparisons to other integers much harder since these multiple cases would need to be handled in each comparison (or a helper function would need to be used, which also isn't that clean).
The other possible alternative to this I see would be a custom enum with u32 and a dedicated MinusOne value. This has the advantage that PartialEq<u32> and PartialOrd<u32> could be implemented for it (and vice versa), so that all comparisons could just work without changing them in any way. This might make sense in general because there are other variables in Ruffle that can just be positive or -1 as well (e.g. frames_loaded which this pull request has also changed (from u16 to i32)).
What do you think about those ideas?
And no, at least I'm not aware of any hard SWF file size cap. I just used i32 because it has previously been u32 and the 2GB are so large that it seemed to make more sense to change it to u32 than u64. The same thought applies to a 4GB file with all the u32s used to store the SWF file size though.
There was a problem hiding this comment.
Would it be possible to store the "this is actually an error SWF" flag in SwfMovie, and then have SwfMovie.uncompressed_len() check for that and return -1 as necessary? (And, of course, not access SwfMovie.header().uncompressed_len directly)
I'm less concerned about the file size cap and more with letting details of how Ruffle works determine swf's interface. If you're doing low-level SWF stuff with swf you only care about the uncompressed file size and not any error conditions that might have been signaled by Flash Player abusing that field.
There was a problem hiding this comment.
That might be a possibility as well. This error condition can only appear if there literally is no existing SWF file though, so in any case where it would be -1, there is no actual file size we could care about.
I can change it to this if you prefer it nevertheless. The only thing worrying me about this is that the identically named functions SwfMovie::uncompressed_len and HeaderExt::uncompressed_len would not always return the same value (although this would be mitigated by documentation). And there are some places where HeaderExt::uncompressed_len is directly used: To create MovieMetadata in web/src/lib.rs and to set the FileResults in scanner/src/execute.rs. I don't know if these values are furtherly used elsewhere and problems arise if these values used directly from the header diverge from the -1 error values retrieved by the MovieClip.
I could change these places to use the SwfMovie::uncompressed_len value, but I'm not sure if it would be good to do that or if there are reasons why they take it from the header directly and maybe should not be -1 in such an error situation.
What would you think about that all?
There was a problem hiding this comment.
I've played around with this for a while and reread your previous statement, and I noticed your "(And, of course, not access SwfMovie.header().uncompressed_len directly)", which I think would probably mean that I should change web/src/lib.rs and scanner/src/execute.rs to use SwfMovie::uncompressed_len instead.
Since my biggest worry was that there were multiple variables containing different values with the same name (uncompressed_len) and that both of these are used in many cases, resulting in two diverging values being used, this would be mitigated by just using SwfMovie::uncompressed_len everywhere (and adding documentation stating that only it should be used).
I still don't really understand what speaks against just directly setting HeaderExt::uncompressed_len to the real Flash value. I personally think that using something like an U32OrMinusOne enum would be a good solution.
In this error case, no SWF file and no "correct" value exists (as there is no file size); the whole header is then just consisting of the error values Flash uses, so in my opinion, having a dedicated error value would be coherent and made sense in the header. An U32OrMinusOne enum allows the correct range of possible values.
And by using this, we could be sure uncompressed_len is always the same and that there aren't diverging variables with the same name, no matter which function is used (or will be added in the future) to get it, so for me, this look a cleaner solution.
However, these are just my thoughts about it. If there are other reasons why you prefer it with a flag, that's fine with me as well. We could also still use such an enum as the HeaderExt::uncompressed_len return value; that would solve the 2GB cap and be a clean way to allow -1 but not other illegal negative values. The only downside to that enum I see is that it needs 8 bytes (as much as an i64) (Edit: but I thought about this, and even this can be improved to 4 bytes with a different U32OrMinusOne struct implementation I thought of).
There was a problem hiding this comment.
Y'know, I think I might be overthinking this. Let's just keep this as-is for now. If it causes a problem later on we can fix it.
| * The following code is unused since Ruffle tests currently don't support tests using remote URLs. | ||
| * It is kept here because Flash behaves differently with online SWFs and offline SWFs. | ||
| * If Ruffle tests start supporting remote URLs (e.g. with a local server) at some point, these tests | ||
| * can still be used. |
There was a problem hiding this comment.
We probably could change the test harness to redirect remote URLs to specific files or simulate specific network conditions. Definitely out of scope for this PR...
There was a problem hiding this comment.
Yeah, Adrian17 said that it would be possible to use a local server or some kind of redirection to simulate tests with remote files.
So if this is done after this PR is merged, we can easily start using those tests as well in another PR. :)
Korne127
force-pushed
the
loader
branch
2 times, most recently
from
b4b1403
to
121f171
Compare
|
I rebased this pull request now and resolved all conflicts. While doing that, I removed the change commenting out the filters test as they're now implemented in AVM1. |
If an SWF tries to load a movie that can't be loaded, the Flash Player sets _framesloaded of the MovieClip to -1. This has previously not been implemented in Ruffle. A new movie_not_available function has been added to the MovieClip class. It sets the cur_preload_frame variable to 0. This function is now called in the movie_loader_error function of the Loader class (if a movie can't be loaded). The frames_loaded function (which returns _framesloaded of the MovieClip, which is given to the SWF) now returns an i32 and not an u16 value. This allows it to return a negative value. Code has been adjusted where an u16 value is expected (clamping negative values to 0). frames_loaded now returns cur_preload_frame - 1. Previously, it has returned cur_preload_frame - 1 and clamped it to zero, but since the -1, in case cur_preload_frame is zero, is explicitly wanted, this has been corrected.
Duplicated functions have been removed (and replaced by corresponding calls to the other functions).
A load_error_swf function has been added to the Loader. It makes the MovieClip enter the error state in which some attributes have certain error values to signal that no valid file could be loaded. This happens if no file could be loaded or if the loaded content is no valid supported content. The function creates an error state movie stub using the new SwfMovie::error_movie function (which uses a new default_error_header function) and configures remaining variables with the movie_not_available method. One TODO in order for the error state to be completely implemented has been added. Since the error state of the MovieClip includes the final URL of the SWF file obtained after any redirects, the load_error_swf and movie_loader_error functions (now) take an swf_url attribute. To get this URL in case no file could be loaded, the NavigatorBackend::fetch method has been changed to return an ErrorResponse struct (including the url and the actual error) in the error case. The Response struct returned in the success case has been renamed to SuccessResponse. All fetch implementations have been adapted accordingly. Code has been adjusted to return the actual error where that's needed. Documentation has been added and improved.
Previously, the MovieClip error state contained one TODO in order to be completely implemented: The HeaderExt variable uncompressed_len needs to be -1 in the error state, but its type has been u32. The type has now been changed to i32, and the variable is set to -1 in the default_error_header function. Therefore, the MovieClip error state is now fully implemented. Other connected functions and variables like SwfMovie::uncompressed_len or MovieClip::total_bytes have been adjusted to this type change. Code has been adjusted if needed where this value is used.
All NavigatorBackend implementations have been refactored, resulting in improved code quality, less duplicated code, more consistent and easier to understand procedures, additional error handling and better error messages. A resolve_url method has been added to the NavigatorBackend trait. It takes a URL and and resolves it to the actual URL from which a file can be fetched (including handling of relative links and pre-processing). It has been implemented in each NavigatorBackend implementation. Duplicated code has been put into new public functions in core/src/backend/navigator.rs which are called by all NavigatorBackend implementations. ExternalNavigatorBackend: - The navigate_to_url and fetch methods have been adapted to use resolve_url, removing redundant code. - Error handling has been added in case that the URL can't be converted to a PathBuf. - A TODO about differences between the flash player fetch and the Ruffle fetch implementation has been added. WebNavigatorBackend: - The previous resolve_url method exclusively to the WebNavigatorBackend has been replaced by the new resolve_url method. It is used by navigate_to_url and fetch. - resolve_url now always pre-processes the URL if it's valid (even if no base_url exists) and explicitly returns whether the URL can be parsed. - navigate_to_url now traces an explanatory error each if the URL can't be parsed or is local. - fetch now returns an explanatory error each if the URL can't be parsed or is local (previously, a vague "Got JS error" has been returned). TestNavigatorBackend & NullNavigatorBackend: - fetch pre-processes the URL now (using the resolve_url implementation). - If the URL isn't local, an explanatory error is returned (previously, it was just an "Invalid URL" error). - If the URL can't be parsed, an explanatory error with the reason is returned (previously, it was just an "Invalid URL" error). Additionally, error messages in all NavigatorBackend implementations have been improved and made more consistent, e.g. if a local file can't be read.
A load_initial_loading_swf function has been added to the Loader. It makes the MovieClip enter the initial loading state in which some attributes have certain initial loading values to signal that the file is currently being loaded and neither an error has occurred nor the first frame has been successfully loaded yet. The Loader calls the function when initialising the loading before it gets an error or success. Additionally, load_error_swf has been improved: If a local URL is fetched on a WASM target, the _url property will now be correctly set as in the flash player. The documentation has been improved.
The set_cur_preload_frame and set_current_frame methods have been added to the MovieClip. If the Loader now loads an image, these methods are called to set the MovieClip image state correctly. Additionally, load_error_swf now uses set_cur_preload_frame instead of the removed movie_not_available method.
Previously, the MovieClip default state contained wrong values for opaqueBackground, transform.pixelBounds and getRect & getBounds with another default MovieClip as the parameter. This has been corrected; the calculation of these values has been fixed. To do that, a use_new_invalid_bounds_value variable has been added to the UpdateContext and the Player (from which the UpdateContext is created). Additionally, an oldest_parent_swf_version variable has been added to the MovieClip with a getter method for it. It is calculated and saved on demand in the getter. These variables are retrieved in the get_bounds function and used to set use_new_invalid_bounds_value and to calculate the correct bounds. Ruffle now uses the correct MovieClip default state.
The behaviour of when the results of getBounds / getRect calls on MovieClips with invalid bounds are 6710886.35 and when they are 6710886.4 for each corner of the rectangle is rather complex. To make sure it works correctly in every case and will continue to do so in the future, eight tests testing the results of getBounds calls under these circumstances have been added. This needs to be tested in several tests because an internal state determining the results can change irreversibly. Making sure that it changes correctly on different occasions takes several tests.
An unload_movie method has been added to the MovieClip. It unloads the MovieClip, which means that one frame after it has been called, avm1_unload and the new transform_to_unloaded_state method get called and the MovieClip enters the unloaded state in which some attributes have certain values. To do this, it creates and spawns a future which calls avm1_unload and transform_to_unloaded_state (which actually makes the MovieClip enter the unloaded state). It uses the new MovieUnloader (which has been added to the Loader enum) to pass the MovieClip to the asynchronous code. The MovieUnloader saves a handle and the target clip, and the asynchronous code retrieves it from the LoadManager. unload_movie is now called when a MovieClip should be unloaded. TODOs have been added, describing to check whether other avm1_unload calls also need to be delayed by one frame and whether unload_movie also needs to be called in other places. Additionally, a spelling mistake has been corrected.
Two tests testing the different states of a MovieClip have been added. A MovieClip can be in different specific states (namely: default state, initial loading state, error state, image state, success state and unloaded state). A state of a MovieClip consists of the values of all properties and the results of some getter functions of the MovieClip. To make sure that all of these states are implemented correctly and will continue to be so in the future, two MovieClip state tests have been added. The movieclip_default_state test tests the default state of a MovieClip after it has been created with createEmptyMovieClip. The movieclip_state_values test has MovieClips going through all other states and tests if the values are correct in each. These tests currently only test the MovieClip states when trying to load a local file since Ruffle tests currently don’t support tests using remote URLs. Test cases for remote URLs are still included and can be used if Ruffle starts supporting remote URLs at some point. A HelperTest, which can be used to find out about how the state of a MovieClip changes when trying to load and unload it, has been included as well.
The suggested changes in the feedback to the pull request have been implemented. Therefore, this commit consists of multiple smaller changes: - The unload_movie method has been renamed to avm1_unload_movie. - The movieclip_default_state test now doesn't test getTextSnapshot().getCount() because the underlying methods haven't been implemented in AVM1 yet. This should be reverted when they have been implemented or stubbed in AVM1.
The MovieClip variable oldest_parent_swf_version and its getter method have been removed because they were unnecessary, as the UpdateContext already provides the root movie. The call of the getter method has been replaced by activation.context.swf.version().
The variable use_new_invalid_bounds has been moved from the UpdateContext and the Player to the Avm1 struct because it is only used for AVM1 code. A getter method and an activator method setting it to true have been added as well. The get_bounds function, which uses the variable, has been adapted to use the getter and the activator. Additionally, documentation has been added and improved. The MovieClip now contains documentation about all MovieClip states and the use_new_invalid_bounds documentation has been extended to explain the logic of how the variable is set and used.
Head branch was pushed to by a user without write access
Lord-McSweeney
removed
the
waiting-on-review
Edit: The scope of this pull request has drastically increased. To read the current summary of it, click here (or scroll down).
If an SWF file tries to load a movie that can't be loaded (e.g. because it doesn't exist), the Flash Player sets _framesloaded of the MovieClip to -1.
This has previously not been implemented in Ruffle (which returned 0).
This caused Paraplüsch / Paraplush to not work in Ruffle and being stuck in the menu. It need loads an additional SWF if you click on a patient and tries two links (an alternative link if the first one doesn't work). Since the original links don't exist anymore and can't be accessed because of CORS, it needs to load the working alternative link, but the error handling code has previously not been invoked due to the false Ruffle behaviour.
This pull request fixes this issue and sets _framesloaded of the MovieClip to -1 if the movie couldn't be loaded.
It also removes some duplicated functions in the code.
However, while testing it, I've noticed a follow-up issue. In Paraplüsch, the movie is loaded vialoadMovie(path, target);. (The code responsible for this loading is inscripts/DefineSprite (1862)/frame 1/DoAction.)But if you replace that function with
loadMovie(path, target, "GET");, loading the patient doesn't work in Ruffle anymore, although it still worked with the flash plugin. (I just noticed this because FFDec automatically does this when recompiling the file.)I tried debugging this, but I don't really know what caused this. It would be good if someone who is familiar with the loadMovie implementation looks over this sometimes. I'll probably open this as an issue if this PR gets merged.
Edit: I noticed this was just an FFDec bug; recompiling the file changed the bytecode and made it not work in both Ruffle and Flash itself (I must have tested the wrong file back when I wrote this). This was unrelated to the "GET".
If someone think that this PR is okay and can be merged, please just tell me and I'll rebase it onto the latest master branch so you can merge it (instead of rebasing using the Github feature). :)