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 767817 - Build error: gstrtpj2kpay.c:364:21: error: implicit truncation from 'int' to bitfield changes value from -1 to 65535
Build error: gstrtpj2kpay.c:364:21: error: implicit truncation from 'int' to ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Mac OS
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-18 15:55 UTC by Francisco Velazquez
Modified: 2016-06-24 12:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Error (498 bytes, text/plain)
2016-06-18 15:55 UTC, Francisco Velazquez
  Details
Don't rely on special tile number value (2.44 KB, patch)
2016-06-21 11:42 UTC, Aaron Boxer
committed Details | Review

Description Francisco Velazquez 2016-06-18 15:55:12 UTC
Created attachment 329999 [details]
Error

gst-plugins-good needs ERROR_CFLAGS='' to build, otherwise the attached error is thrown in Mac OS X.
Comment 1 Tim-Philipp Müller 2016-06-20 09:25:40 UTC
Aaron, any chance you could have a look?

For what it's worth, I'm not sure we really need a bitfield for the header struct, so could just change the types perhaps? (In case we parse any bits from incoming data, it should be parsed via GstBitReader and assigned to the struct, not indirectly picked up like that)
Comment 2 Aaron Boxer 2016-06-20 11:17:06 UTC
I am using -1 to indicate invalid tile number, but there is already another field to indicate whether the tile number is valid. So, just setting it to 0 should be fine.
Comment 3 Aaron Boxer 2016-06-20 11:20:07 UTC
Actually, let me take a closer look
Comment 4 Tim-Philipp Müller 2016-06-20 11:28:07 UTC
I only had a quick glance and it looked like some code tried to distinguish between 0 invalid and -1 not set.

In any case, assigning -1 to a guint bitfield is not right and the compiler rightly complains about it.
Comment 5 Aaron Boxer 2016-06-20 12:42:15 UTC
(In reply to Tim-Philipp Müller from comment #4)
> I only had a quick glance and it looked like some code tried to distinguish
> between 0 invalid and -1 not set.
> 
> In any case, assigning -1 to a guint bitfield is not right and the compiler
> rightly complains about it.

Quite right. Simply remove the bitfield signifier from the "tile" member, and it should all be good.
Comment 6 Aaron Boxer 2016-06-20 15:05:15 UTC
For such a simple change, I did not attach a patch.
Comment 7 Sebastian Dröge (slomo) 2016-06-21 08:21:57 UTC
Aaron, can you attach a patch? Thanks :)
Comment 8 Tim-Philipp Müller 2016-06-21 08:37:15 UTC
I just assigned 0xffff now.

commit 323244bc047c26d3027e3424804859e0abd9a8a0
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Tue Jun 21 09:34:56 2016 +0100

    rtpj2kpay: fix compiler warning on OS/X
    
    gstrtpj2kpay.c:364:21: error: implicit truncation from 'int' to bitfield changes value from -1 to 65535
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767817
Comment 9 Aaron Boxer 2016-06-21 10:37:09 UTC
Tim, the standard actually allows images with 655536 tiles.
So, I think we ought to change bitfield to int. I will make a patch :)
Comment 10 Tim-Philipp Müller 2016-06-21 10:44:37 UTC
Yes, but that's besides the point.

The "the-tile-field-is-invalid" flag will have to be set anyway.

I did not see any code that actually checks for the value and does stuff based on that, so the validity must be tracked separately already, but if it's not my patch should not have any adverse effect at least.

I don't see how changing it to -1 changes anything or makes anything better. It will still be written out as 0xffff.
Comment 11 Aaron Boxer 2016-06-21 11:19:20 UTC
(In reply to Tim-Philipp Müller from comment #10)
> Yes, but that's besides the point.
> 
> The "the-tile-field-is-invalid" flag will have to be set anyway.
> 
> I did not see any code that actually checks for the value and does stuff
> based on that, so the validity must be tracked separately already, but if
> it's not my patch should not have any adverse effect at least.
> 
> I don't see how changing it to -1 changes anything or makes anything better.
> It will still be written out as 0xffff.

Thanks. I need to differentiate the very first tile part using -1. Will make this more explicit.
Comment 12 Aaron Boxer 2016-06-21 11:20:03 UTC
Also, I didn't see your patch when pull the latest from master.
Comment 13 Aaron Boxer 2016-06-21 11:31:58 UTC
woops, looking at wrong project :)
Comment 14 Aaron Boxer 2016-06-21 11:42:51 UTC
Created attachment 330127 [details] [review]
Don't rely on special tile number value

This fixed the logic for detecting multiple tiles, so we don't rely on the special tile number -1, which was causing compile errors on OSX
Comment 15 Tim-Philipp Müller 2016-06-21 12:06:47 UTC
Comment on attachment 330127 [details] [review]
Don't rely on special tile number value

Thanks Aaron!

commit f07c704b49f599812799105f6b621ece120eb18c
Author: Aaron Boxer <boxerab@gmail.com>
Date:   Tue Jun 21 07:40:42 2016 -0400

    gstrtpj2kpay: use tile bit and tile number to determine if there are multiple tiles in packet
    
    Now we don't have to rely on a special value for the tile number.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767817
Comment 16 Aaron Boxer 2016-06-22 17:27:19 UTC
(In reply to Tim-Philipp Müller from comment #15)
> Comment on attachment 330127 [details] [review] [review]
> Don't rely on special tile number value
> 
> Thanks Aaron!
> 

Glad to help out.
Comment 17 Aaron Boxer 2016-06-24 12:04:26 UTC
By the way, now that the project is on Github, have you considered using Travis CI? It can be configured to build on OSX VMs, so it would have caught this compile error.
Comment 18 Sebastian Dröge (slomo) 2016-06-24 12:34:44 UTC
We have our own CI that is running jenkins and doing builds and various platforms, hard to do the same in a meaningful way with Travis.

It was not caught there because we compile with -Wno-error there... because Apple compilers cause too many false positives and differ too much from version to version about what they complain about.
Comment 19 Aaron Boxer 2016-06-24 12:45:03 UTC
(In reply to Sebastian Dröge (slomo) from comment #18)
> We have our own CI that is running jenkins and doing builds and various
> platforms, hard to do the same in a meaningful way with Travis.
> 
> It was not caught there because we compile with -Wno-error there... because
> Apple compilers cause too many false positives and differ too much from
> version to version about what they complain about.

Good to know, thanks for the explanation :)