Is animation#1255
Conversation
Fix false positives where plain strings containing '=' were treated as animated properties. - replace the naive "contains '='" check with keyframe-aware parsing - require an unquoted key=value token whose key parses as a frame/time token - support optional single-character keyframe-type markers before '=' (e.g. |, !, ~, $, -, and easing markers A-D/a-z) - ignore separators inside quoted payloads so normal string values remain valid - document the updated detection rules in the API comment This preserves valid animation strings while preventing non-animated text (such as URL/query-like values) from being misclassified as animation.
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent plain strings (notably URLs containing = in query parameters) from being misclassified as animation strings, which can lead to unintended parsing/corruption when properties are accessed via animation-aware getters.
Changes:
- Adds a regression test ensuring a URL containing
=is not treated as animated. - Updates
mlt_property_is_anim()to detect animation more selectively by scanning for unquotedkey=valuetokens where the key resembles a frame/time token. - Adjusts string handling in
mlt_property_anim_get_string()to strip protective wrapping quotes and cache the unquoted value without mutating the underlying serialized string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/test_animation/test_animation.cpp | Adds a regression test for URLs containing = to ensure they are not treated as animation strings. |
| src/framework/mlt_property.c | Refines animation detection and adds quoted-string unquoting/cache behavior in mlt_property_anim_get_string(). |
| if (!in_quotes && *p == '=') | ||
| return is_keyframe_token(token_start, p); | ||
| if (!in_quotes && *p == ';') | ||
| token_start = p + 1; |
There was a problem hiding this comment.
I disagree with this. "foo=bar;50=100" should not be an animation. What does "foo=bar" mean? is it just junk that should be ignored? Not a bug.
There was a problem hiding this comment.
Agreed. It should definitely return immediately if false, and "foo" is not a valid time value--short circuit. But this does not continue validating items if is_keyframe_token() is true. IOW, does `mlt_property_is_anim("50=100;foo=bar") return true? I consider that an animation even though it is not entirely valid. If it walks and talks like an animation; it is an animation. OTOH something like "Love = everything; you = my world" is not an animation.
There was a problem hiding this comment.
"50=100;foo=bar"
I could add a test for that if we want it to work. In that case, "foo=bar" will just be considered junk and the end of the animation parsing. For example, "50=100;foo=bar;60=120" would be the same as "50=100" because the "foo=bar" would cause the parsing to abort. In addition to the extra test for that, I would also update the documentation to explain:
One leading valid key/value pair is enough to consider the animation valid. However, if invalid key/value pairs are encountered during parsing, the parsing will abort and only the initial valid keyframes will be interpreted. If the first characters of the string do not make up a valid key/value pair, then the entire string is not interpreted to be an animation and is considered a string.
Does that seem good?
There was a problem hiding this comment.
"50=100;foo=bar;60=120" would be the same as "50=100
That is my expectation. I think a unit test is desirable.
There was a problem hiding this comment.
This idea conflicts with an existing unit test:
https://github.com/mltframework/mlt/blob/master/src/tests/test_animation/test_animation.cpp#L355
The current behavior for this test string is that the parser assigns the frame to 0 and uses that if a frame/time can not be parsed from the string token.
So, with the current behavior, 50=100;foo=bar;60=120 becomes
0=foo=bar
50=100
60=120
It is easy to change the existing test. Do we want to do that?
There was a problem hiding this comment.
The spirit of that sub-test is "anim strings may contain equal signs if quoted" and your change keeps that. That test is an extreme outlier case. Change the test to add a frame key to resolve it.
AboutGMIC has a note with a url that was getting corrupted due to having an equal sign and being interpreted as an animation. This is one proposed fix for that. Another option would be to use the non-animation functions for strings in the openfx module.