Kdenlive   bug tracker Home page

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0002182KdenliveMLTpublic2011-06-21 09:552011-07-07 07:18
ReporterAlexey Osipov 
Assigned Toj-b-m 
PrioritynormalSeveritymajorReproducibilityalways
StatusacknowledgedResolutionopen 
Platform64 bitOSUbuntuOS Version10.04
Product VersionRecent git 
Target VersionFixed in Version 
Summary0002182: '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 InformationProposed patch to fix this:

--- a/src/framework/mlt_geometry.c
+++ b/src/framework/mlt_geometry.c
@@ -26,6 +26,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <locale.h>
 
 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[4] = 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;
             count ++;
         }
+
+ // Set old locale back
+ setlocale(LC_NUMERIC, old_locale);
     }
     else
     {
TagsNo tags attached.
Build/Install Method3rd party package
Attached Filespng file icon screenshot1.png [^] (328,848 bytes) 2011-06-21 09:55

- Relationships

-  Notes
(0007048)
j-b-m (administrator)
2011-07-06 22:58

Problem aknowledged on the forum by another user, see:

http://kdenlive.org/forum/problem-sliders [^]
(0007049)
ddennedy (developer)
2011-07-07 00:02

[frame=]X,Y:WxH[:mix][;[frame=]X,Y:WxH[:mix]]
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:

[frame=]X/Y/W/H[/mix][;[frame=]X/Y/W/H[/mix]]

I see in mlt_geometry.c:mlt_geometry_serialise() will need to be changed. Is Kdenlive taking advantage of that or Mlt::Geometry::serialise()?
(0007050)
ddennedy (developer)
2011-07-07 00:15

or how about "frame='X/Y:WxH:mix;" ?
(0007051)
j-b-m (administrator)
2011-07-07 00:29

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...
(0007052)
ddennedy (developer)
2011-07-07 00:38

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.
(0007053)
j-b-m (administrator)
2011-07-07 01:40

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?
(0007056)
ddennedy (developer)
2011-07-07 03:06

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.
(0007058)
ddennedy (developer)
2011-07-07 07:18

The main offenders in MLT should be fixed in git commits 50b0cf and 70245c.

- Issue History
Date Modified Username Field Change
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 - 2014 MantisBT Team
Powered by Mantis Bugtracker