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)

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.

Actually, let me take a closer look

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.

(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.

For such a simple change, I did not attach a patch.

Aaron, can you attach a patch? Thanks :)

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

Tim, the standard actually allows images with 655536 tiles.
So, I think we ought to change bitfield to int. I will make a patch :)

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.

(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.

Also, I didn't see your patch when pull the latest from master.

woops, looking at wrong project :)

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

(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.

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.

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.

(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 :)