After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 691945 - _cogl_unpack_uint{8,16}_t
_cogl_unpack_uint{8,16}_t
Status: RESOLVED FIXED
Product: cogl
Classification: Platform
Component: general
git master
Other NetBSD
: Normal normal
: ---
Assigned To: Cogl maintainer(s)
Cogl maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-01-17 12:41 UTC by Patrick Welche
Modified: 2013-01-21 15:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
bitmap: Don't try to token paste the typenames from stdint.h (26.54 KB, patch)
2013-01-18 18:21 UTC, Neil Roberts
none Details | Review

Description Patrick Welche 2013-01-17 12:41:15 UTC
_cogl_unpack_uint{8,16}_t are called from cogl-bitmap-conversion.c, but where are they defined?
Comment 1 Neil Roberts 2013-01-17 13:13:43 UTC
They are defined in cogl-bitmap-packing.h which is a file which gets included multiple times with different values for some macros in order to generate macros for the different types. The names are generated by token pasting the type name which is why the definitions can't be found with grep.

Is this causing a problem for you?
Comment 2 Patrick Welche 2013-01-17 13:25:45 UTC
Trying to build cogl-1.12.0, I get as far as

  CCLD   test-conformance
../../cogl/.libs/libcogl.so: undefined reference to `_cogl_pack_uint8_t'
../../cogl/.libs/libcogl.so: undefined reference to `_cogl_pack_uint16_t'
../../cogl/.libs/libcogl.so: undefined reference to `_cogl_unpack_uint8_t'
../../cogl/.libs/libcogl.so: undefined reference to `_cogl_unpack_uint16_t'

hence the question. The library compiles correctly, so the definition must have been seen then, the breakage is at the linking stage.

(BTW to get that far, I had to

 # General warning about experimental features
-if test "x$EXPERIMENTAL_CONFIG" = "xyes"; then
+if test "x$EXPERIMENTAL_CONFIG" = "xXXX"; then

as the characters broke my xterm - maybe something less fancy would be more portable? Maybe after I get cogl built, I can have a fancier terminal ;-) - and Bug 691944.

Thanks for the pointer - I'll have a rummage...
Comment 3 Patrick Welche 2013-01-18 17:22:38 UTC
I don't understand the following:

# nm -g cogl/.libs/cogl-bitmap-conversion.o
                 U _GLOBAL_OFFSET_TABLE_
0000000000000a09 T _cogl_bitmap_convert
0000000000000671 T _cogl_bitmap_convert_into_bitmap
                 U _cogl_bitmap_copy_subregion
                 U _cogl_bitmap_map
                 U _cogl_bitmap_new_with_malloc_buffer
0000000000000492 T _cogl_bitmap_premult
                 U _cogl_bitmap_set_format
                 U _cogl_bitmap_unmap
00000000000002ae T _cogl_bitmap_unpremult
                 U _cogl_context_get_default
                 U _cogl_pack_uint16_t
                 U _cogl_pack_uint8_t
                 U _cogl_unpack_uint16_t
                 U _cogl_unpack_uint8_t
                 U cogl_bitmap_get_format
                 U cogl_bitmap_get_height
                 U cogl_bitmap_get_rowstride
                 U cogl_bitmap_get_width
                 U cogl_object_unref
                 U g_assertion_message
                 U g_free
                 U g_malloc

That suggests that _cogl_pack_uint16_t was not defined in cogl-bitmap-conversion.o, and yet
_cogl_pack_ ## component_type
should have happened in cogl/cogl-bitmap-conversion.c by the method you describe in Comment 1. (gcc 4.5.4)
Comment 4 Neil Roberts 2013-01-18 17:44:39 UTC
The function is declared as static inline so it makes sense that it's not defined in cogl-bitmap-conversion.o. What doesn't make sense is that there would be anything referring to the symbol because any reference to it from cogl-bitmap-conversion.c should just end up inlining it. There shouldn't be any other references to it from any other files.

If I build cogl-1.12.0 and run nm -g cogl/.libs/cogl-bitmap-conversion.o then I don't get any mention of the _cogl_pack_* functions which is what I would expect. I can't explain why cogl-bitmap-conversion.o would be trying to link to those symbols. Do you get any warnings or anything when you build it?
Comment 5 Neil Roberts 2013-01-18 17:48:49 UTC
Maybe you could try compiling the file with -E to look at the preprocessor output and verify that it is correctly generating the function definitions in cogl-bitmap-conversion.c.
Comment 6 Neil Roberts 2013-01-18 17:52:24 UTC
What platform are you building for?

Perhaps in your system headers uint8_t and friends are #define'd to something instead of being typedefs. That would certainly break it.
Comment 7 Neil Roberts 2013-01-18 18:20:40 UTC
If that is the case then the following patch might fix it:
Comment 8 Neil Roberts 2013-01-18 18:21:06 UTC
Created attachment 233782 [details] [review]
bitmap: Don't try to token paste the typenames from stdint.h

Previously the functions for packing and unpacking pixels where
generated by token pasting together a function name along with its
type, like the following:

 _cogl_pack_ ## uint8_t

Then later in cogl-bitmap-conversion.c it would directly refer to the
function names without token pasting.

This wouldn't work however if the system headers define the stdint
types using #defines instead of typedefs because in that case the
function name generated using token pasting would get the expanded
type name but the reference that doesn't use token pasting wouldn't.

This patch adds an extra macro passed to the cogl-bitmap-packing.h
header which just has the type size. That way the function can be
defined like this instead:

 _cogl_pack_ ## 8

That should prevent it from hitting problems with #defined types.
Comment 9 Patrick Welche 2013-01-19 12:48:51 UTC
It seems you beat me to it, and with a more elegant patch :-)

In my sys/types.h:
#ifndef uint8_t
typedef __uint8_t       uint8_t;
#define uint8_t         __uint8_t
#endif

So cogl-bitmap-conversion.c ends up defining

_cogl_unpack___uint8_t (CoglPixelFormat format,
                                         const __uint8_t *src,
                                         __uint8_t *dst,
                                         int width)
but calling _cogl_unpack_uint8_t, and the workaround patch I used to compile has lines of the form

--- cogl/cogl-bitmap-conversion.c.orig  2012-09-17 10:43:00.000000000 +0000
+++ cogl/cogl-bitmap-conversion.c
@@ -430,9 +430,9 @@ _cogl_bitmap_convert_into_bitmap (CoglBi
       dst = dst_data + y * dst_rowstride;
 
       if (use_16)
-        _cogl_unpack_uint16_t (src_format, src, tmp_row, width);
+        G_PASTE (_cogl_unpack_, uint16_t) (src_format, src, tmp_row, width);
       else
-        _cogl_unpack_uint8_t (src_format, src, tmp_row, width);
+        G_PASTE (_cogl_unpack_, uint8_t) (src_format, src, tmp_row, width);
 
       /* Handle premultiplication */
       if (need_premult)

Adding a new component_size looks neater. Thank you!
Comment 10 Patrick Welche 2013-01-19 13:39:53 UTC
I can now successfully compile cogl with the patches in Bug 692077, Bug 691944 and your patch.
Comment 11 Robert Bragg 2013-01-21 14:04:55 UTC
(In reply to comment #8)
> Created an attachment (id=233782) [details] [review]
> bitmap: Don't try to token paste the typenames from stdint.h
> 
> Previously the functions for packing and unpacking pixels where
> generated by token pasting together a function name along with its
> type, like the following:
> 
>  _cogl_pack_ ## uint8_t
> 
> Then later in cogl-bitmap-conversion.c it would directly refer to the
> function names without token pasting.
> 
> This wouldn't work however if the system headers define the stdint
> types using #defines instead of typedefs because in that case the
> function name generated using token pasting would get the expanded
> type name but the reference that doesn't use token pasting wouldn't.
> 
> This patch adds an extra macro passed to the cogl-bitmap-packing.h
> header which just has the type size. That way the function can be
> defined like this instead:
> 
>  _cogl_pack_ ## 8
> 
> That should prevent it from hitting problems with #defined types.

This patch looks good to land to me
Comment 12 Neil Roberts 2013-01-21 15:13:03 UTC
Ok, I've pushed this one to master and the 1.12 branch. Thanks.