Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ndusart
Copy link

@ndusart ndusart commented Aug 26, 2024

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
Copy link
Owner

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.

@pr0gramista pr0gramista self-assigned this Aug 29, 2024
@ndusart
Copy link
Author

ndusart commented Aug 30, 2024

@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 😮

@pr0gramista
Copy link
Owner

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
Copy link
Author

ndusart commented Aug 30, 2024

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 😕 ❓

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants