search

pr0gramista / charset_converter

Flutter platform charset converter
BSD 3-Clause "New" or "Revised" License
29 stars 19 forks source link

remove unnecessary appending of null bytes in FlValue as length is known #42

Open ndusart opened 3 weeks ago

ndusart commented 3 weeks ago

Hi,

This PR removes a useless copy of the input string when decoding in Linux because the glib function (g_convert_with_iconv) actually does not expect a C-string but an array of gchar with the actual length (length may be set to -1 for null-terminated string).

When obtaining the string on the C side:

    FlValue *data = fl_value_lookup_string(args, "data");
    gchar *toUse = (gchar *)fl_value_get_uint8_list(data);
    gsize charlen = fl_value_get_length(data);

charlen will effectively encode the string length so toUse does not require to end with a null byte :)

pr0gramista commented 2 weeks ago

Hi, thanks for the PR 💙

I was intrigued whether I left this without a proper check, but it seems like the PR would break the decoding with the following error:

** (charset_converter_example:28711): CRITICAL **: 13:29:26.584: const uint8_t *fl_value_get_uint8_list(FlValue *): assertion 'self->type == FL_VALUE_TYPE_UINT8_LIST' failed
** (charset_converter_example:28711): CRITICAL **: 13:29:26.584: size_t fl_value_get_length(FlValue *): assertion 'self->type == FL_VALUE_TYPE_UINT8_LIST || self->type == FL_VALUE_TYPE_INT32_LIST || self->type == FL_VALUE_TYPE_INT64_LIST || self->type == FL_VALUE_TYPE_FLOAT32_LIST || self->type == FL_VALUE_TYPE_FLOAT_LIST || self->type == FL_VALUE_TYPE_LIST || self->type == FL_VALUE_TYPE_MAP' failed

Tested on Flutter 3.24.1 on Ubuntu 24.01.1 LTS 6.8.0-41-generic.

You can run tests using flutter test integration_test/app_test.dart from the example app. Let me know if I missed something.

ndusart commented 2 weeks ago

@pr0gramista on my side this test successfully pass with this commit (Flutter 3.24.1, arch linux 6.10.6-arch1-1)

The asserts are strange because on Dart-side, the argument data is a Uint8List already:

static Future<String> decode(String charset, Uint8List data)

So fl_value_get_uint8_list and fl_value_get_length should accept the argument.

I'm a bit clueless about why there is a divergence between our runs :open_mouth:

pr0gramista commented 2 weeks ago

That is indeed interesting, maybe the time has come for my first Arch system.

To help me narrow down the issue, could you tell me more about your system? Architecture and version of GTK would be helpful to compare.

ndusart commented 2 weeks ago

This does not seem related to GLib but more to how flutter itself convert the data to the native side.

My architecture is x86_64.

Here is a list of the libraries linked to example binary:

$ ldd build/linux/x64/debug/bundle/charset_converter_example 
    linux-vdso.so.1 (0x00007e4d5ad8d000)
    libcharset_converter_plugin.so => /home/nicolas/dev/github/charset_converter/example/build/linux/x64/debug/bundle/lib/libcharset_converter_plugin.so (0x00007e4d5ad79000)
    libflutter_linux_gtk.so => /home/nicolas/dev/github/charset_converter/example/build/linux/x64/debug/bundle/lib/libflutter_linux_gtk.so (0x00007e4d58400000)
    libgtk-3.so.0 => /usr/lib/libgtk-3.so.0 (0x00007e4d57c00000)
    libgdk-3.so.0 => /usr/lib/libgdk-3.so.0 (0x00007e4d57b14000)
    libz.so.1 => /usr/lib/libz.so.1 (0x00007e4d57afb000)
    libharfbuzz.so.0 => /usr/lib/libharfbuzz.so.0 (0x00007e4d579e1000)
    libpangocairo-1.0.so.0 => /usr/lib/libpangocairo-1.0.so.0 (0x00007e4d5ad67000)
    libpango-1.0.so.0 => /usr/lib/libpango-1.0.so.0 (0x00007e4d57978000)
    libatk-1.0.so.0 => /usr/lib/libatk-1.0.so.0 (0x00007e4d57951000)
    libcairo.so.2 => /usr/lib/libcairo.so.2 (0x00007e4d5781e000)
    libcairo-gobject.so.2 => /usr/lib/libcairo-gobject.so.2 (0x00007e4d583b7000)
    libgdk_pixbuf-2.0.so.0 => /usr/lib/libgdk_pixbuf-2.0.so.0 (0x00007e4d577da000)
    libgio-2.0.so.0 => /usr/lib/libgio-2.0.so.0 (0x00007e4d5760c000)
    libglib-2.0.so.0 => /usr/lib/libglib-2.0.so.0 (0x00007e4d574bd000)
    libgobject-2.0.so.0 => /usr/lib/libgobject-2.0.so.0 (0x00007e4d5745e000)
    libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x00007e4d57000000)
    libm.so.6 => /usr/lib/libm.so.6 (0x00007e4d5736f000)
    libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x00007e4d57341000)
    libc.so.6 => /usr/lib/libc.so.6 (0x00007e4d56e0f000)
    libdl.so.2 => /usr/lib/libdl.so.2 (0x00007e4d5733c000)
    libepoxy.so.0 => /usr/lib/libepoxy.so.0 (0x00007e4d56d02000)
    libfontconfig.so.1 => /usr/lib/libfontconfig.so.1 (0x00007e4d572ec000)
    libpthread.so.0 => /usr/lib/libpthread.so.0 (0x00007e4d572e5000)
    /lib64/ld-linux-x86-64.so.2 => /usr/lib64/ld-linux-x86-64.so.2 (0x00007e4d5ad8f000)
    libgmodule-2.0.so.0 => /usr/lib/libgmodule-2.0.so.0 (0x00007e4d572de000)
    libpangoft2-1.0.so.0 => /usr/lib/libpangoft2-1.0.so.0 (0x00007e4d572c2000)
    libfribidi.so.0 => /usr/lib/libfribidi.so.0 (0x00007e4d572a2000)
    libXi.so.6 => /usr/lib/libXi.so.6 (0x00007e4d5728f000)
    libX11.so.6 => /usr/lib/libX11.so.6 (0x00007e4d56bc1000)
    libatk-bridge-2.0.so.0 => /usr/lib/libatk-bridge-2.0.so.0 (0x00007e4d56b85000)
    libcloudproviders.so.0 => /usr/lib/libcloudproviders.so.0 (0x00007e4d56b6c000)
    libtracker-sparql-3.0.so.0 => /usr/lib/libtracker-sparql-3.0.so.0 (0x00007e4d56a95000)
    libXfixes.so.3 => /usr/lib/libXfixes.so.3 (0x00007e4d56a8d000)
    libxkbcommon.so.0 => /usr/lib/libxkbcommon.so.0 (0x00007e4d56a45000)
    libwayland-client.so.0 => /usr/lib/libwayland-client.so.0 (0x00007e4d56a36000)
    libwayland-cursor.so.0 => /usr/lib/libwayland-cursor.so.0 (0x00007e4d56a2c000)
    libwayland-egl.so.1 => /usr/lib/libwayland-egl.so.1 (0x00007e4d57286000)
    libXext.so.6 => /usr/lib/libXext.so.6 (0x00007e4d56a17000)
    libXcursor.so.1 => /usr/lib/libXcursor.so.1 (0x00007e4d56a0b000)
    libXdamage.so.1 => /usr/lib/libXdamage.so.1 (0x00007e4d56a06000)
    libXcomposite.so.1 => /usr/lib/libXcomposite.so.1 (0x00007e4d569ff000)
    libXrandr.so.2 => /usr/lib/libXrandr.so.2 (0x00007e4d569f2000)
    libXinerama.so.1 => /usr/lib/libXinerama.so.1 (0x00007e4d569ed000)
    libfreetype.so.6 => /usr/lib/libfreetype.so.6 (0x00007e4d56923000)
    libgraphite2.so.3 => /usr/lib/libgraphite2.so.3 (0x00007e4d56901000)
    libthai.so.0 => /usr/lib/libthai.so.0 (0x00007e4d568f4000)
    libpng16.so.16 => /usr/lib/libpng16.so.16 (0x00007e4d568ba000)
    libXrender.so.1 => /usr/lib/libXrender.so.1 (0x00007e4d568ae000)
    libxcb.so.1 => /usr/lib/libxcb.so.1 (0x00007e4d56883000)
    libxcb-render.so.0 => /usr/lib/libxcb-render.so.0 (0x00007e4d56874000)
    libxcb-shm.so.0 => /usr/lib/libxcb-shm.so.0 (0x00007e4d5686f000)
    libpixman-1.so.0 => /usr/lib/libpixman-1.so.0 (0x00007e4d567c3000)
    libjpeg.so.8 => /usr/lib/libjpeg.so.8 (0x00007e4d56727000)
    libtiff.so.6 => /usr/lib/libtiff.so.6 (0x00007e4d5669c000)
    libmount.so.1 => /usr/lib/libmount.so.1 (0x00007e4d5664d000)
    libpcre2-8.so.0 => /usr/lib/libpcre2-8.so.0 (0x00007e4d565ae000)
    libffi.so.8 => /usr/lib/libffi.so.8 (0x00007e4d565a1000)
    libexpat.so.1 => /usr/lib/libexpat.so.1 (0x00007e4d56578000)
    libatspi.so.0 => /usr/lib/libatspi.so.0 (0x00007e4d56542000)
    libdbus-1.so.3 => /usr/lib/libdbus-1.so.3 (0x00007e4d564f1000)
    libjson-glib-1.0.so.0 => /usr/lib/libjson-glib-1.0.so.0 (0x00007e4d564c7000)
    libxml2.so.2 => /usr/lib/libxml2.so.2 (0x00007e4d56379000)
    libsqlite3.so.0 => /usr/lib/libsqlite3.so.0 (0x00007e4d5620d000)
    libbz2.so.1.0 => /usr/lib/libbz2.so.1.0 (0x00007e4d561fa000)
    libbrotlidec.so.1 => /usr/lib/libbrotlidec.so.1 (0x00007e4d561eb000)
    libdatrie.so.1 => /usr/lib/libdatrie.so.1 (0x00007e4d561e2000)
    libXau.so.6 => /usr/lib/libXau.so.6 (0x00007e4d561db000)
    libXdmcp.so.6 => /usr/lib/libXdmcp.so.6 (0x00007e4d561d3000)
    libzstd.so.1 => /usr/lib/libzstd.so.1 (0x00007e4d560f4000)
    liblzma.so.5 => /usr/lib/liblzma.so.5 (0x00007e4d560c1000)
    libjbig.so.2.1 => /usr/lib/libjbig.so.2.1 (0x00007e4d560b3000)
    libblkid.so.1 => /usr/lib/libblkid.so.1 (0x00007e4d56078000)
    libsystemd.so.0 => /usr/lib/libsystemd.so.0 (0x00007e4d55f84000)
    libicuuc.so.75 => /usr/lib/libicuuc.so.75 (0x00007e4d55d8a000)
    libbrotlicommon.so.1 => /usr/lib/libbrotlicommon.so.1 (0x00007e4d55d67000)
    libcap.so.2 => /usr/lib/libcap.so.2 (0x00007e4d55d5b000)
    libicudata.so.75 => /usr/lib/libicudata.so.75 (0x00007e4d54000000)

GTK is 3.24.43 and GLib is 2.80.5.

I created a VM running Ubuntu 24.04.1 LTS (uname -a: 6.8.0-41-generic #41-Ubuntu SMP PREEMPT_DYNAMIC Fri Aug 2 20:41:06 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux) and the test passes :confused: :question:

Does the test passes on your system for the master branch ?