- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Hello,
We have an application in which we use UIC to process JPEG and PNG images, and we came across a number of issues.
1. Crash on unsupported subsamplings
We have in OwnJPEGDecoder::ReadHeader() :
geometry.SetEnumSampling(j2u_smap[m_sampling]);
When decoding a JPEG image with an unsupported subsampling, SetEnumSampling() is called with SOther, however it returns an error, leaving ImageSamplingGeometry::m_sampleSize untouched, with all values at 0. This will go unnoticed as EnumSampling() will return S444, all values being equal. Then, when calling ImageBuffer::ReAlloc(), we run into:
Rect ComponentRect(Ipp32u component) const {
return Decimate(m_refGridRect, m_sampleSize[component]);
}
We end up with a division by 0, and a crash. This is tricky as we can't simply check that EnumSampling() != SOther, since even after being set to SOther it wrongly returns S444.
2. Wrong buffer size calculations
When trying to decode in planar mode a JPEG image featuring vertical subsampling (for example YUV 420), ImageSamplingGeometry::m_sampleSize is populated with 2x2 for the Y plane and 1x1 for the U and V plane. Then, if I understand correctly, when allocating the plane memory buffers, their size are calculated in ImageBufferFormat::NOfBytes() with:
DataOrder().LineStep()[component] * SamplingGeometry().ComponentRect(component).Height()
Now once again, it seems to me that ComponentRect() essentially performs a division by the sample size. So for the Y plane, the height is divided by 2, and a half-sized buffer is allocated; whereas for U and V, the height is divided by 1 so a full-sized buffer is allocated.
I think the intent of the code is to allocate reduced-sized buffer for subsampled planes that take less space. However, if if my analysis is right, not only memory is wasted of U and V, but a buffer that is too small is used for Y. Then during decoding, the buffer is overrun causing a crash (and it indeed crashes for me). I must be understanding something wrong or missing something here, because this is way too blatant.
3. Unimplemented 244 decoding
If the above issue is fixed, decoding S244 images to YUV planes still crashes, in CJPEGDecoder::ProcessBuffer(), in ippiCopy_8u_C1R(). A quick reading of the code shows that there are three if statements, for planes found in S444, S411 and S422, but none for S244; so apparently, in the case of U and V planes of S244, data is never initialized and former data from the Y plane is used, leading to a segmentation fault somewhere down the line.
4. CheckByte() bounds check
I'm delighted to see that you fixed the CBitStreamInput::CheckByte() issue, because we were hit by that too. I don't know if it's considered an issue but if CheckByte() is called with a value that is negative or bigger than m_DataLen, it may cause an out-of-bounds buffer access.
5. Rewind on EOD
There is another bug that hit us in this piece of code, failing the parsing of some JPEG images. Some code in jpegdec.cpp seeks backwards. Most of the time it works fine, the corner case is when FillBuffer() already reached the end of the data and rewinding the stream requires rebuffering: nothing resets m_eod so FillBuffer() returns an error. This kind of patch works around the issue:
6. Planar mode PNG calls
Trying to decode/encode a PNG image in planar mode will result in a crash, because the code simply assumes Interleaved. A check that returns an error would probably be preferable.
7. PNG memory leak
I see that the PNG setjmp() issue (DPD200214139) was fixed. But it seems to me that there's still a memory leak on an error path in the encoder:
8. Encoding setup errors
When encoding a JPEG image from a planar format, an error is returned if chroma conversion is requested (or if there is a single plane). Yet I think that could work at least for RGB -> YUV. But the real problem is that these errors are returned when trying to merely set the input and output parameters. For 3 planes, calling SetParams() before AttachImage() returns an error because the specified destination format is found different from the uninitialized source one... For 4 planes, both CJPEGEncoder::SetSource() and CJPEGEncoder::SetParams() return an error if input and output formats are different, so it's just impossible to set either first, and at all. This makes it impossible to properly set up encoding of a YCCK image.
9. Missing bugfixes
I read in the changelog of version 7.0.5 that the new ImageSamplingGeometry::Period() function was fixed. However, looking at the sources in the tarball, I see no change from last version.
10. 2-component IPP functions
Sometimes we come across PNG grayscale + alpha images, so 2 components, and I'm at a loss because I'd normally use ippiResizeSqrPixel_8u_C2R() to resize them but there is no such function. Nor is there an ippiReduceBits_16u8u_C2R() which I could use too.
I hope you can help with a couple of these :)
We have an application in which we use UIC to process JPEG and PNG images, and we came across a number of issues.
1. Crash on unsupported subsamplings
We have in OwnJPEGDecoder::ReadHeader() :
geometry.SetEnumSampling(j2u_smap[m_sampling]);
When decoding a JPEG image with an unsupported subsampling, SetEnumSampling() is called with SOther, however it returns an error, leaving ImageSamplingGeometry::m_sampleSize untouched, with all values at 0. This will go unnoticed as EnumSampling() will return S444, all values being equal. Then, when calling ImageBuffer::ReAlloc(), we run into:
Rect ComponentRect(Ipp32u component) const {
return Decimate(m_refGridRect, m_sampleSize[component]);
}
We end up with a division by 0, and a crash. This is tricky as we can't simply check that EnumSampling() != SOther, since even after being set to SOther it wrongly returns S444.
2. Wrong buffer size calculations
When trying to decode in planar mode a JPEG image featuring vertical subsampling (for example YUV 420), ImageSamplingGeometry::m_sampleSize is populated with 2x2 for the Y plane and 1x1 for the U and V plane. Then, if I understand correctly, when allocating the plane memory buffers, their size are calculated in ImageBufferFormat::NOfBytes() with:
DataOrder().LineStep()[component] * SamplingGeometry().ComponentRect(component).Height()
Now once again, it seems to me that ComponentRect() essentially performs a division by the sample size. So for the Y plane, the height is divided by 2, and a half-sized buffer is allocated; whereas for U and V, the height is divided by 1 so a full-sized buffer is allocated.
I think the intent of the code is to allocate reduced-sized buffer for subsampled planes that take less space. However, if if my analysis is right, not only memory is wasted of U and V, but a buffer that is too small is used for Y. Then during decoding, the buffer is overrun causing a crash (and it indeed crashes for me). I must be understanding something wrong or missing something here, because this is way too blatant.
3. Unimplemented 244 decoding
If the above issue is fixed, decoding S244 images to YUV planes still crashes, in CJPEGDecoder::ProcessBuffer(), in ippiCopy_8u_C1R(). A quick reading of the code shows that there are three if statements, for planes found in S444, S411 and S422, but none for S244; so apparently, in the case of U and V planes of S244, data is never initialized and former data from the Y plane is used, leading to a segmentation fault somewhere down the line.
4. CheckByte() bounds check
I'm delighted to see that you fixed the CBitStreamInput::CheckByte() issue, because we were hit by that too. I don't know if it's considered an issue but if CheckByte() is called with a value that is negative or bigger than m_DataLen, it may cause an out-of-bounds buffer access.
5. Rewind on EOD
There is another bug that hit us in this piece of code, failing the parsing of some JPEG images. Some code in jpegdec.cpp seeks backwards. Most of the time it works fine, the corner case is when FillBuffer() already reached the end of the data and rewinding the stream requires rebuffering: nothing resets m_eod so FillBuffer() returns an error. This kind of patch works around the issue:
[plain]diff -urNp ipp-samples/image-codecs/uic/src/codec/image/jpeg/dec/src/bitstreamin.cpp ipp-samples/image-codecs/uic/src/codec/image/jpeg/dec/src/bitstreamin.cpp
--- ipp-samples/image-codecs/uic/src/codec/image/jpeg/dec/src/bitstreamin.cpp 2011-07-07 09:23:10.000000000 +0200
+++ ipp-samples/image-codecs/uic/src/codec/image/jpeg/dec/src/bitstreamin.cpp 2011-09-13 18:24:52.000000000 +0200
@@ -76,6 +76,11 @@ JERRCODE CBitStreamInput::Detach(void)
JERRCODE CBitStreamInput::Init(int bufSize)
{
+ // Minimum size to support rewind on EOD
+ if(bufSize < DEC_DEFAULT_BUFLEN)
+ {
+ bufSize = DEC_DEFAULT_BUFLEN;
+ }
m_DataLen = (int)bufSize;
if(0 != m_pData)
@@ -150,6 +155,7 @@ JERRCODE CBitStreamInput::Seek(UIC::Base
{
_offset = m_currPos + (int)offset;
+ // XXX Should be _offset >= 0
if(_offset > 0 && _offset < m_DataLen)
{
m_currPos = _offset;
@@ -161,6 +167,13 @@ JERRCODE CBitStreamInput::Seek(UIC::Base
if(UIC::BaseStream::StatusOk != status)
return JPEG_ERR_FILE;
+ // Implement rewind on EOD
+ if(_offset <= 0 && m_eod)
+ {
+ m_eod = 0;
+ if(m_DataLen < DEC_DEFAULT_BUFLEN)
+ m_currPos = m_DataLen = DEC_DEFAULT_BUFLEN;
+ }
jerr = FillBuffer();
if(JPEG_OK != jerr)
return jerr;[/plain] 6. Planar mode PNG calls
Trying to decode/encode a PNG image in planar mode will result in a crash, because the code simply assumes Interleaved. A check that returns an error would probably be preferable.
7. PNG memory leak
I see that the PNG setjmp() issue (DPD200214139) was fixed. But it seems to me that there's still a memory leak on an error path in the encoder:
[plain]diff -urNp ipp-samples/image-codecs/uic/src/codec/image/png/enc/src/uic_png_enc.cpp ipp-samples/image-codecs/uic/src/codec/image/png/enc/src/uic_png_enc.cpp
--- ipp-samples/image-codecs/uic/src/codec/image/png/enc/src/uic_png_enc.cpp 2011-07-07 09:23:09.000000000 +0200
+++ ipp-samples/image-codecs/uic/src/codec/image/png/enc/src/uic_png_enc.cpp 2011-09-13 18:20:52.000000000 +0200
@@ -209,6 +209,7 @@ public:
if(m_iHeight > IPP_MAX_32U)
{
+ delete []pRow;
png_destroy_write_struct(&m_pPNGStruct, &m_pInfo);
return ExcStatusFail;
}[/plain] 8. Encoding setup errors
When encoding a JPEG image from a planar format, an error is returned if chroma conversion is requested (or if there is a single plane). Yet I think that could work at least for RGB -> YUV. But the real problem is that these errors are returned when trying to merely set the input and output parameters. For 3 planes, calling SetParams() before AttachImage() returns an error because the specified destination format is found different from the uninitialized source one... For 4 planes, both CJPEGEncoder::SetSource() and CJPEGEncoder::SetParams() return an error if input and output formats are different, so it's just impossible to set either first, and at all. This makes it impossible to properly set up encoding of a YCCK image.
9. Missing bugfixes
I read in the changelog of version 7.0.5 that the new ImageSamplingGeometry::Period() function was fixed. However, looking at the sources in the tarball, I see no change from last version.
10. 2-component IPP functions
Sometimes we come across PNG grayscale + alpha images, so 2 components, and I'm at a loss because I'd normally use ippiResizeSqrPixel_8u_C2R() to resize them but there is no such function. Nor is there an ippiReduceBits_16u8u_C2R() which I could use too.
I hope you can help with a couple of these :)
Link Copied
2 Replies
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Hello again,
Will you fix any of these issues at all in the next release? :(
Also, it appears to me that the JPEG decoder isn't very error-resilient, it will return an error when a file is truncated or somewhat corrupted, where web browsers usually manage to display something. In particular, an otherwise correct file missing only the 0xffd9 End Of Image marker will return an error; I am told that such JPEG files are not uncommon, due to bad encoders omitting the EOI marker, so it would be nice to support them (and/or truncated files).
Will you fix any of these issues at all in the next release? :(
Also, it appears to me that the JPEG decoder isn't very error-resilient, it will return an error when a file is truncated or somewhat corrupted, where web browsers usually manage to display something. In particular, an otherwise correct file missing only the 0xffd9 End Of Image marker will return an error; I am told that such JPEG files are not uncommon, due to bad encoders omitting the EOI marker, so it would be nice to support them (and/or truncated files).
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
Regarding those errors that look too obvious, believe me they are.
I was the person who reported Period() function errors so at least I can help you with that.
There are two errors:
1. You need to initialize minimums at the beginning to UINT_MAX instead of 0.
2. You need to correct copy&paste induced error involving W and H.
There is also a lot of inefficient code (cases and if's) which set a bad example for new developers looking at the code and using it as a template but since samples are not a part of the product that seems to be ignored for now.
I was the person who reported Period() function errors so at least I can help you with that.
There are two errors:
1. You need to initialize minimums at the beginning to UINT_MAX instead of 0.
2. You need to correct copy&paste induced error involving W and H.
There is also a lot of inefficient code (cases and if's) which set a bad example for new developers looking at the code and using it as a template but since samples are not a part of the product that seems to be ignored for now.
Reply
Topic Options
- Subscribe to RSS Feed
- Mark Topic as New
- Mark Topic as Read
- Float this Topic for Current User
- Bookmark
- Subscribe
- Printer Friendly Page