Error in DIB_PAD_BYTES in UIC picnic sample 6.1.3.052

[Thomas_Jensen1]

The macro DIB_PAD_BYTES in jpegbase.h is declared as DIB_PAD_BYTES(width,nchannels).

However, the argument nchannels is actually npixelbytes.

I found this bug when I found a problem with a particular Dicom file with a JpegLL 12 bit grayscale image with an uneven width. The width is 2039 pixels.

In jpeg.cpp, the pixel data of an image is allocated when decoding, using function CIppImage::Alloc() in ippimage.cpp.

The code in Alloc() passes nchannels to DIB_PAD_BYTES, whereas it should actuall pass npixelbytes.

So, for uneven widths, decoding fails with AV or a corrupt heap.

Bad code:

int CIppImage::Alloc(IppiSize roi, int nchannels, int precision, int align)
{
int size;
int newStep = roi.width * nchannels * (((precision & 255)+7)/8);
if(align)
newStep += DIB_PAD_BYTES(roi.width,nchannels);

Corrected code:

int CIppImage::Alloc(IppiSize roi, int nchannels, int precision, int align)
{
int size;
int channelbytes = ((precision & 255)+7)/8;
int newStep = roi.width * nchannels * channelbytes;
if(align)
newStep += DIB_PAD_BYTES(roi.width,nchannels * channelbytes);

Specifically, for a width of 2039, the aligned newStep becomes 4079, which is wrong (should be 4080, evenly divisible by 4 bytes).

Finally, it would be better to rename DIB_PAD_BYTES(width,nchannels) to DIB_PAD_BYTES(width,npixelbytes)

[Chao_Y_Intel]

Hi Thomas,

Thank you. I will forward it to the engineering owner for future fix.

Thanks,

Chao

[Sergey_Ryadno]

Hi Thomas,

thanks for reporting issue. i invistigated this sitation and make fixes according you post.

[Chao_Y_Intel]

This fix is included in the IPP 6.1 update 5 package.

Thanks,
Chao

[Thomas_Jensen1]

I want to comment on this, so that you may clean up the sample code a bit, since it also serves as documentation.

The new code in IPP 6.1 update 5:

#define DIB_UWIDTH(width,nchannels) \
((width) * (nchannels))

#define DIB_AWIDTH(width,nchannels) \
( ((DIB_UWIDTH(width,nchannels) + DIB_ALIGN) & (~DIB_ALIGN)) )

#define DIB_PAD_BYTES(width,nchannels) \
( DIB_AWIDTH(width,nchannels) - DIB_UWIDTH(width,nchannels) )


Although the code computes properly, the parameter nchannels is wrongly named. It is actually pixelbytes.
Motivation: if image is 8u_C1, then DIB_UWIDTH(10,1) is correct at result = 10. If image is 16u_C1, then DIB_UWIDTH(10,1) is incorrect at result = 10 (should have been 20).

When renaming the parameter pixelbytes (or any other similar name), then it adds up.
Motivation: if image is 8u_C1, then DIB_UWIDTH(10,1) is correct at result = 10. If image is 16u_C1, then DIB_UWIDTH(10,2) is correct at result = 20.

In fact, code in file ippimage.cpp uses DIB_PAD_BYTES a lot, and it does in fact pass pixel size as the second parameter.

There is also a seemingly duplicate function in file uic_bmp_enc.cpp:
static int DIB_PAD_BYTES(
int width,
int nchannels).


While I'm at it, there are two ippimage.cpp files:
in picnic
in uic_transcoder_con
The file in uic_transcoder_con seems to be more modern.

In addition:
The code in jpegenc.cpp is still incorrect:
buflen = (m_jpeg_mode == JPEG_LOSSLESS) ?
IPP_MAX(ENC_DEFAULT_BUFLEN,m_numxMCU * m_jpeg_ncomp * 2) :
ENC_DEFAULT_BUFLEN;

I'm I'm not wrong, it should be:
buflen = (m_jpeg_mode == JPEG_LOSSLESS) ?
IPP_MAX(ENC_DEFAULT_BUFLEN,m_numxMCU * m_jpeg_ncomp * 2 * 2) :
ENC_DEFAULT_BUFLEN;

Also, this code in uic_new.h is wrong:
inline void operator delete(void*, size_t, const UIC::NewBuffer &) {}

I'm I'm not wrong, it should be:
inline void operator delete(void*, const UIC::NewBuffer &) {}