VXL file format differs with 64 bit build and 32 bit build

[philips-thilo]

The VXL file format differs for 64 bit builds and 32 bit builds. This is due to the usage of size_t and sizeof(size_t) in Frame.h. While for a 64 bit compiler size_t will be 8 bytes, it will be 4 bytes for a 32 bit build.

The problem is that this is not checked and if a file is being read, either 4 bytes of the frame data will be interpreted as size (32 bit vxl-file on a 64 bit build) or you will get a 4 byte offset for the frame data (64 bit vxl-file on a 32 bit build). In case of reading 32 bit vxl-files with a 64 bit build of the SDK, the software will crash.

[hlprasu]

I don't think it is because of size_t. It must be due "int" type for "id" in Frame class. Replacing "int" will "int64_t" should solve this issue. Please check.

On 20 July 2016 at 14:08, Thilo Enters notifications@github.com wrote:

The VXL file format differs for 64 bit builds and 32 bit builds. This is due to the usage of size_t and sizeof(size_t) in Frame.h. While for a 64 bit compiler size_t will be 8 bytes, it will be 4 bytes for a 32 bit build.

The problem is that this is not checked and if a file is being read, either 4 bytes of the frame data will be interpreted as size (32 bit vxl-file on a 64 bit build) or you will get a 4 byte offset for the frame data (64 bit vxl-file on a 32 bit build). In case of reading 32 bit vxl-files with a 64 bit build of the SDK, the software will crash.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/3dtof/voxelsdk/issues/85, or mute the thread https://github.com/notifications/unsubscribe-auth/AE0wjPKupoA6XRWAAuMe3UwUBPs34Mfxks5qXd5xgaJpZM4JQjLA .

~ Prasad Bhat

[hlprasu]

Sorry, size_t is also an issue. "id" is another issue for this. Replace size_t with uint64_t apart from the above replacement for "id" variable.

On 20 July 2016 at 14:21, Prasad Bhat hlprasu@gmail.com wrote:

I don't think it is because of size_t. It must be due "int" type for "id" in Frame class. Replacing "int" will "int64_t" should solve this issue. Please check.

On 20 July 2016 at 14:08, Thilo Enters notifications@github.com wrote:

The VXL file format differs for 64 bit builds and 32 bit builds. This is due to the usage of size_t and sizeof(size_t) in Frame.h. While for a 64 bit compiler size_t will be 8 bytes, it will be 4 bytes for a 32 bit build.

The problem is that this is not checked and if a file is being read, either 4 bytes of the frame data will be interpreted as size (32 bit vxl-file on a 64 bit build) or you will get a 4 byte offset for the frame data (64 bit vxl-file on a 32 bit build). In case of reading 32 bit vxl-files with a 64 bit build of the SDK, the software will crash.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/3dtof/voxelsdk/issues/85, or mute the thread https://github.com/notifications/unsubscribe-auth/AE0wjPKupoA6XRWAAuMe3UwUBPs34Mfxks5qXd5xgaJpZM4JQjLA .

~ Prasad Bhat

~ Prasad Bhat

[philips-thilo]

id should not be an issue, as it is the same as int32_t on most of the systems - so replacing it with int32_t should at least be a little bit cleaner.

Replacing size_t with uint64_t was also my first idea, but it does not make a lot of sense, as the superseeding FrameStreamPacket stores uint32_t as size (see https://github.com/3dtof/voxelsdk/blob/master/Voxel/DataPacket.h line 24 and https://github.com/3dtof/voxelsdk/blob/master/Voxel/DataPacket.cpp lines 19 and 35). So if you use uin64_t in the Frame class hierarchy, you might violate the container format DataPacket.

[hlprasu]

It might be better to replace size type in DataPacket to uint64_t, with the primary risk of breaking backward compatibility with previous .vxl files. Say this can be done in a phased manner later.

But, replacing size_t with uint64_t should make previous .vxl files (recorded in 64-bit platform), readable in 32-bit platform.

On 20 July 2016 at 15:02, Thilo Enters notifications@github.com wrote:

id should not be an issue, as it is the same as int32_t on most of the systems - so replacing it with int32_t should at least be a little bit cleaner.

Replacing size_t with uint64_t was also my first idea, but it does not make a lot of sense, as the superseeding FrameStreamPacket stores uint32_t as size (see https://github.com/3dtof/voxelsdk/blob/master/Voxel/DataPacket.h:24 and https://github.com/3dtof/voxelsdk/blob/master/Voxel/DataPacket.cpp:19 and :35). So if you use uin64_t in the Frame class hierarchy, you might violate the container format DataPacket.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/3dtof/voxelsdk/issues/85#issuecomment-233899927, or mute the thread https://github.com/notifications/unsubscribe-auth/AE0wjFI7dy7r0Ao4sXcOiWC6EzIs-DU_ks5qXesegaJpZM4JQjLA .

~ Prasad Bhat

[philips-thilo]

I am not sure whether it makes sense to replace the size in DataPacket to uint64_t. Will there ever be a packet that contains more than 4 GB of data? 32 bit systems will always have to emulate 64 bit because of their smaller registers without any benefit.

Breaking backward compatiblity: Isn't there a file revision in the header? Then I think it is time to change it, as the format of the binary file is changing. We don't have to argue about compatibility here, because VXL with header version major 0, minor 1 is ambiguous and does not contain an endianness definition either. Additionally reading the 64 bit recordings on a 32 bit machine leads to corrupted data that you can hardly make any use of, see here: https://e2e.ti.com/support/sensor/optical_sensors/f/989/t/530197 As the SDK does not have any check for major and minor revision yet, old versions of the SDK are likely to fail to read newer files one day anyway.

One solution might be:

1. For files with major revision == 0 handle every size_t as uint64_t
2. For files with major revision > 0 handle every size_t as uint32_t

Edit: All in all my prefered solution would be: Have a clean definition and implementation of bitwidth and major/minor handling and break compatibility now to avoid compatiblity issues in the future.

[hlprasu]

Sorry for late reply. I agree regarding revision. Remember that revision is at top level of VXL format and it needs to percolated to the level of each data packet parsing.

It might become messy to support earlier revision and new revision with size_t redefinition as this would have to done in run-time and not compile time.

I would recommend keeping it as uint64_t itself and based on revision read/write it like uint32_t or uint64_t. Since it is appears just once in one packet it should hardly impact in anyway for run-time or storage.

Regarding the link, I believe that with this fix, all data recorded in 64-bit platform (assuming same endianess) will be readable in 32-bit platform as is.

On 21 July 2016 at 13:12, Thilo Enters notifications@github.com wrote:

I am not sure whether it makes sense to replace the size in DataPacket to uint64_t. Will there ever be a packet that contains more than 4 GB of data? 32 bit systems will always have to emulate 64 bit because of their smaller registers without any benefit.

Breaking backward compatiblity: Isn't there a file revision in the header? Then I think it is time to change it, as the format of the binary file is changing. We don't have to argue about compatibility here, because VXL with header version major 0, minor 1 is ambiguous and does not contain an endianness definition either. Additionally reading the 64 bit recordings on a 32 bit machine leads to corrupted data that you can hardly make any use of, see here: https://e2e.ti.com/support/sensor/optical_sensors/f/989/t/530197 http://url

One solution might be:

1. For files with revision == 0.1 handle every size_t as uint64_t
2. For files with a revision > 0.1 handle every size_t as uint32_t

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/3dtof/voxelsdk/issues/85#issuecomment-234180504, or mute the thread https://github.com/notifications/unsubscribe-auth/AE0wjIoAJjkXYu6U_MAikrfH5efa7Fkiks5qXyLugaJpZM4JQjLA .

~ Prasad Bhat

[philips-thilo]

Hi Prasad,

no problem, I also took some time off.

Remember that revision is at top level of VXL format and it needs to percolated to the level of each data packet parsing.

Yes, I know that and usually you would not want to do that. You can also avoid it by always just supporting the current version, if you do not want to support downwards compatibility.

It might become messy to support earlier revision and new revision with size_t redefinition as this would have to done in run-time and not compile time.

I agree, as indicated above. At this point I am not sure whether it makes sense to introduce that compatibility here. But at least you should break compatibility with older files to avoid crashes. An intentional error message is much nicer than "segmentation fault", "access violation" or "stopped responding"... Additionally these old files can be altered to be compatible with a new format, see https://github.com/3dtof/voxelsdk/pull/86

I would recommend keeping it as uint64_t itself and based on revision read/write it like uint32_t or uint64_t. Since it is appears just once in one packet it should hardly impact in anyway for run-time or storage.

I still disagree. Does it make sense to have support for data packets of 4GB+? I currently do not expect that from the application context. Although it is correct that it will hardly impact storage or run-time, it still breaks compatibility. So basically I see it as a design-decision as no change here will provide a general fix. And design-based, as I do not expect data packets in a frame stream for the environment of depth cameras that exceed the size of 4GB, I would still go for uint32.

Regarding the link, I believe that with this fix, all data recorded in 64-bit platform (assuming same endianess) will be readable in 32-bit platform as is.

Yes, I tested that, that is why I proposed that command-line based processing that only needs to be done once in a batch.