Unpacking 12 bit pixel data into 16 bit data type

[hinxx]

I'm trying out the implementation of the 12 bit data unpacking https://github.com/areaDetector/ADGenICam/blob/c227e7770efebd98bb912e839226ecc826903e99/GenICamApp/src/ADGenICam.cpp#L555-L559.

It seems that the result will be 12 bits of data occupying bits 15:4 of the uint16 data type, and with 3:0 zeroed out. This is comparable to the Mono16 data type that these cameras are able to produce. My user working with FLIR BFS-PGE-70S7M would prefer to see unpacked 12 bits rather occupy 11:0 of the uint16 data type, with 15:12 zeroed out. On our side, we are assuming that the pixel data should always be non-negative; is this appropriate to expect?

I'm using this IOC startup directive:

dbLoadRecords("NDStdArrays.template", "P=$(PREFIX),R=image1:,PORT=Image1,ADDR=0,TIMEOUT=1,NDARRAY_PORT=$(PORT),TYPE=Int16,FTVL=SHORT,NELEMENTS=$(NELEMENTS)")

One thing that bothered my user was having bit 15 used for pixel data, with 12 bit relevant payload. This resulted in EPICS clients getting waveform with signed 16 bit values where bit 15 would flip the sign interpreted by Matlab as negative pixel value. His Matlab code would have to treat arriving data as int16 then shift the pixel value 4 bits to the right to get into 12 range and avoid 'negative' value pixel values. Maybe I'm doing something wrong but, I can not get the waveform to hold unsigned 16 bit values; it seems that asyn layer with the asynInt16 data type is the reason. IOW, changing FTVL=SHORT to FTVL=USHORT has no effect. Is this expected? I know that going 32 bits from the IOC -> DB -> client makes this a non-issue but it feels an overkill in data overhead, for my taste at least.

Is the use of higher bits a standard way of transporting the pixels (i.e. do other devices do this)?

With the current implementation how does using IOC "shift left" PV with 12 bit data type result in valid pixel values? I guess it should be always set to 'no' shift, but then again, it is possible to perform the shift only if the pixel format is not 8 or 16 bits.

For reference, here is the change that makes my user happy:

diff --git a/GenICamApp/src/ADGenICam.cpp b/GenICamApp/src/ADGenICam.cpp
index 5f84c7d..870b9fe 100755
--- a/GenICamApp/src/ADGenICam.cpp
+++ b/GenICamApp/src/ADGenICam.cpp
@@ -553,8 +553,10 @@ void ADGenICam::decompressMono12Packed(int numPixels, epicsUInt8 *input, epicsUI
     int i;

     for (i=0; i<numPixels/2; i++) {
-        *output++ = (*input << 8) | ((*(input+1) & 0x0f) << 4);
-        *output++ = (*(input+1) & 0xf0) | (*(input+2) << 8);
+        *output++ = ((epicsUInt16)((*input << 4) | (*(input+1) & 0x0f))) & 0x0FFF;
+        *output++ = ((epicsUInt16)(((*(input+1) & 0xf0) >> 4) | (*(input+2) << 4))) & 0x0FFF;
         input += 3;
     }
 }

With this the EPICS clients get to see lowest 12 bits of the 16 bit data type occupied with pixel data. No more negative pixel values. A (nice) side effect is that, for example, the statistics plugin is now showing proper pixel values in 12 bit range. Also, Matlab code does not have to do any data manipulation either as pixel can not get negative. An OPI with intensity XY plot also looks better now.

What do you think?

[MarkRivers]

@hinxx I knew this issue would come up, I just didn't think it would be so soon!

You did not say which driver you are using, ADAravis or ADSpinnaker?

I implemented these unpack functions so they would produce the same data as Mono16 on FLIR cameras as you remarked. It also produces the same Mono12Packed as ADSpinnaker, because the Spinnker SDK unpacks with the left shift. But I agree that this is not always what the user wants. I suggest we have 2 versions of decompressMono12Packed and decompressMono12p.

• decompressMono12PackedHigh Bits 0-3 are 0
• decompressMono12PackedLow Bits 12-15 are 0
• decompressMono12pHigh Bits 0-3 are 0
• decompressMono12pLow Bits 12-15 are 0

We would add a new PV, ConvertPixelFormat with choices Mono16High or Mono16Low. Note that this PV already exists for ADSpinnaker and ADAravis, but does not currently have those choices.

Currently ADSpinnaker and ADVimba use the vendor SDK to convert Mono12Packed, For ADSpinnaker this means the data is always left-shifted. But we could change ADVimba and ADSpinnaker to use the above functions for Mono12Packed and Mono12p so the user also has a choice with those.

[hinxx]

You did not say which driver you are using, ADAravis or ADSpinnaker?

I'm using ADAravis.

We would add a new PV, ConvertPixelFormat with choices Mono16High or Mono16Low

This sounds like a nice solution.

Let me know if I can help in any way!

[MarkRivers]

I have pushed new versions of ADGenICam and ADAravis to address this issue.

The R1-7 ADGenICam release notes: https://github.com/areaDetector/ADGenICam/blob/master/RELEASE.md explain the changes to ADGenICam.

The R2-2 ADAravis release notes https://github.com/areaDetector/ADAravis/blob/master/RELEASE.md explain the changes to ADAravis. These changes allow selecting whether bit-shifting is done for Mono12Packed and Mono12p. They also allow complete control over bit shifting for 16-bit data: no shift, left shift, right shift, and number of bits to shift.

[MarkRivers]

Note that I have not tagged those versions of ADGenICam and ADAravis. You need to use the master branch to test those new features.

[hinxx]

After initial tests the change works as advertised.

I was getting:

2020/10/08 10:26:20.265 ADAravis:writeInt32 error, status=3 function=111 ARAVIS_CONVERT_PIXEL_FORMAT, value=1
2020/10/08 10:26:21.715 ADAravis:writeInt32 error, status=3 function=111 ARAVIS_CONVERT_PIXEL_FORMAT, value=0

A small code change fixed it:

diff --git a/aravisApp/src/ADAravis.cpp b/aravisApp/src/ADAravis.cpp
index 48333e0..38a84a9 100644
--- a/aravisApp/src/ADAravis.cpp
+++ b/aravisApp/src/ADAravis.cpp
@@ -560,7 +560,7 @@ asynStatus ADAravis::writeInt32(asynUser *pasynUser, epicsInt32 value)
     } else if (function == AravisConnection) {
         if (this->connectionValid != 1) status = asynError;
     } else if (function == AravisFrameRetention || function == AravisPktResend || function == AravisPktTimeout ||
-               function == AravisShiftDir || function == AravisShiftBits) {
+               function == AravisShiftDir || function == AravisShiftBits || function == AravisConvertPixelFormat) {
         /* just write the value for these as they get fetched via getIntegerParam when needed */
     } else if ((function < FIRST_ARAVIS_CAMERA_PARAM) || (function > LAST_ARAVIS_CAMERA_PARAM)) {
         /* If this parameter belongs to a base class call its method */

Did notice that if shift direction is not None and 12 bit format is selected, the pixel values are additionally shifted; you noted that shift direction should be None in that case and it is up to the user to get it right.

I wonder if code should enforce the no shift direction since the decompressor handles that. Something like this might do the trick (not tested):

diff --git a/aravisApp/src/ADAravis.cpp b/aravisApp/src/ADAravis.cpp
index 48333e0..a5f9ff6 100644
--- a/aravisApp/src/ADAravis.cpp
+++ b/aravisApp/src/ADAravis.cpp
@@ -766,6 +766,7 @@ asynStatus ADAravis::processBuffer(ArvBuffer *buffer) {
         //printf("Time to convert Mono12 = %f\n", epicsTimeDiffInSeconds(&tend, &tstart));
         size = width * height * sizeof(epicsUInt16);
         releaseArray = true;
+        shiftDir = AravisShiftNone;
     }
     //  Print the first 8 pixels of the buffer in decimal
     //for (int i=0; i<8; i++) printf("%u ", ((epicsUInt16 *)pRaw->pData)[i]); printf("\n");

Thanks!

[MarkRivers]

A small code change fixed it:

I have made that change, thanks.

I wonder if code should enforce the no shift direction since the decompressor handles that.

I think it is good to allow additional shifting in the case of Mono12Packed and Mono12p. The reason is that those formats do not mean that the data is actually 12 bits. For example on my FLIR Oryx camera the mode I often run is AdcBitDepth=10, Mono12Packed because that lets me run at 130 frames/s, compared to 89 frames/s with AdcBitDepth=12. If I use those settings with ConvertPixelFormat=Mono16Low then the data values are all a multiple of 4, i.e. bits 0-1 are 0. In order for the image data to be truly 10-bit I need to shift right an additional 2 bits.

I have updated the release notes to discuss this: https://github.com/areaDetector/ADAravis/blob/master/RELEASE.md