|Anonymous | Login||2016-07-27 13:46 CEST|
|My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0002182||Kdenlive||MLT||public||2011-06-21 09:55||2011-07-07 07:18|
|Platform||64 bit||OS||Ubuntu||OS Version||10.04|
|Product Version||Recent git|
|Target Version||Fixed in Version|
|Summary||0002182: 'Composite' and possible other transitions incorrectly parse 'geometry' value in non-C locales|
|Description||'geometry' property of 'Composite' transition given to mlt_geometry.c:201 have format "[frame=]X,Y:WxH[:mix][;[frame=]X,Y:WxH[:mix]]*"|
Affecting part of a format is "X,Y".
In some non-C locales (including ru_RU) ',' character is used as decimal separator.
So, when mlt_geometry.c:331 call strtod(), it eats "X,Y" part of a format as ONE double value. Hence, all other tokens of a format become shifted by one: W becomes Y, H becomes W, and mix becomes H. :) Therefore, user have very strange result like in attached screenshot.
|Additional Information||Proposed patch to fix this:|
@@ -26,6 +26,7 @@
typedef struct geometry_item_s
@@ -284,6 +285,7 @@
int mlt_geometry_parse_item( mlt_geometry self, mlt_geometry_item item, char *value )
int ret = 0;
+ char* old_locale = NULL;
// Get the local/private structure
geometry g = self->local;
@@ -324,6 +326,9 @@
item->f = 1;
+ // for strtod stop on ','
+ old_locale = setlocale(LC_NUMERIC, "C");
// Iterate through the remainder of value
while( *value )
@@ -372,6 +377,9 @@
value = p;
+ // Set old locale back
+ setlocale(LC_NUMERIC, old_locale);
|Tags||No tags attached.|
|Build/Install Method||3rd party package|
|Attached Files||screenshot1.png [^] (328,848 bytes) 2011-06-21 09:55|
Problem aknowledged on the forum by another user, see:
is only a _convention_ that needs to be revised - not introduce an ugly hack in the code that makes it locale-insensitive!
mlt_geometry.c does not require , as a delimiter. The only delimiters it requires are semi-colon, equal sign, and anything non-numeric according to LC_NUMERIC. Kdenlive needs to provide numbers according to the correct locale and start using a different format for geometry. I suggest something that is also friendly to bash. Here is an example:
I see in mlt_geometry.c:mlt_geometry_serialise() will need to be changed. Is Kdenlive taking advantage of that or Mlt::Geometry::serialise()?
|or how about "frame='X/Y:WxH:mix;" ?|
Ok, so if I understand, you will change the mlt_geometry_serialise_cut to that new format. Will the old format [frame=]X,Y:WxH[:mix][;[frame=]X,Y:WxH[:mix]] still be understood?
For me "frame='X/Y:WxH:mix;" is ok. I will check the Kdenlive code, because I think we do some string manipulations on that...
A user also reported problems with the locale numeric separator with the Bézier curves effects, but I guess this is unrelated, either related to the frei0r_helper or the frei0r Bézier effect...
Yes, it is backwards compatible. I will commit the change very soon.
P.S. I knew about this problem and that the code comment and demo examples were bad examples contributing to it. I had been meaning to change them, but when I looked into it further today I realized mlt_geometry_serialise() was another problem. It will be good to finally take care of it.
Wow, I am just now discovering that this locale numeric separator is breaking lots of things. For example, in our effects .xml files, we have stuff like that to define default values:
<parameter type="list" name="Channel" default="0.5" paramlist="0.5,0,0.1,0.2,0.3,0.4,0.6,0.71">
So we have to change our separator for the paramlist, but even with that, the values are not be interpreted correctly with a french locale, as it would require a comma as separator. All parameters get a value of 0.
The same problem happens in MLT, for example in the vignette effects (oldfilm/filter_vignette.c), line 125:
mlt_properties_set( MLT_FILTER_PROPERTIES( this ), "smooth", "0.8" );
The smooth value is not correctly interpreted with a french locale!
Anybody has an idea about how to solve that nightmare?
XML and similar documents (including MLT XML) should declare their locale. Then, a processor should setlocale() or similar when converting its extracted numeric strings. In the context of Kdenlive, MLT XML really only needs that in order to exchange project files between users of differing locales, which I had considered a lower priority enhancement; however, I am willing to address it now.
For the effects definition XML, since you completely control it, it is fine for you to set LC_NUMERIC=C. However, you need to make sure numeric string values do not get passed straight through to libraries. IOW, when parsing the effects XML set LC_NUMERIC=C and store values with numeric primitives. Then, restore the locale. Finally, supply the values to MLT using locale-sensitive toString conversions or the numeric property setters. MLT does not have the same liberty to setlocale - people use melt command lines assuming their locale (or should) or generate MLT XML with a tool running under a specific locale that is converting their numeric primitives to proper, locale-sensitive strings.
Default MLT property values can be changed easily enough to use better functions like mlt_properties_set_double() (numeric property setter). I will add that to the list of changes for this bug.
|The main offenders in MLT should be fixed in git commits 50b0cf and 70245c.|
|2011-06-21 09:55||Alexey Osipov||New Issue|
|2011-06-21 09:55||Alexey Osipov||File Added: screenshot1.png|
|2011-07-06 22:58||j-b-m||Note Added: 0007048|
|2011-07-06 22:58||j-b-m||Assigned To||=> j-b-m|
|2011-07-06 22:58||j-b-m||Status||new => acknowledged|
|2011-07-07 00:02||ddennedy||Note Added: 0007049|
|2011-07-07 00:15||ddennedy||Note Added: 0007050|
|2011-07-07 00:29||j-b-m||Note Added: 0007051|
|2011-07-07 00:38||ddennedy||Note Added: 0007052|
|2011-07-07 01:40||j-b-m||Note Added: 0007053|
|2011-07-07 03:06||ddennedy||Note Added: 0007056|
|2011-07-07 07:18||ddennedy||Note Added: 0007058|
|Copyright © 2000 - 2016 MantisBT Team|