3

Closed

Failed assertion in EncodeStrategy

description

Hi,
 
we received a bug report for the copy of CharLS that is part of the DCMTK:
http://forum.dcmtk.org/viewtopic.php?f=1&t=3412
 
I took a look at this file and I think that this is indeed a bug in CharLS.
 
The attached patch creates an EncoderStrategy instance and writes bits to it. This triggers the same assertion as the original test image. (Ignore my changes to encoderstrategy.h, that's just the debug output that I looked at for figuring this out):
 
encoderstrategy.h:95: void EncoderStrategy::AppendToBitStream(LONG, LONG): Assertion `bitpos >=0' failed.
 
My analsysis of what is going on is part of the patch, so be sure to read the comments. :-)
 
I tried to come up with a patch that fixes this. This is the following part of the diff:
 
  • if (bitpos >= 0)
  • if (bitpos > 0)
     
    The idea here is that when 32 bits of input were read, we can Flush().
     
    This should even fix a similar issue, where we have bitpos == 1 and add 31 input bits, so one bit gets added to valcurrent. If Flush() only writes out 30 bits, all the 30 bits of input left can go into the buffer.
     
    Cheers,
    Uli

file attachments

Closed May 27, 2016 at 11:46 PM by vbaderks
fixed in the github repository.

comments

psychon wrote May 7, 2012 at 11:19 AM

I just noticed that the suggested fix causes undefined behavior according to the C++ standard. AppendToBitStream() needs another patch.

Before:
    Flush();

    ASSERT(bitpos >=0);
    valcurrent |= value = 0);
    if (bitpos != 32)
        valcurrent |= value << bitpos;
}
Sorry for not noticing this earlier.

jdv wrote May 13, 2012 at 7:28 PM

Hi Uli,

Thanks for noticing this.. as well as the unit test.
That one was long overdue!

The funny thing is I got a similar bug report earlier this week.
When this happens, is the image still lossless and decodable?

Also what version of charls are you using?
I am thinking of making a new 1.1 release based on the current trunk.

psychon wrote May 14, 2012 at 6:41 AM

Hi,

Sorry, but what do you mean with "When this happens, is the image still lossless and decodable?"? The failed assertion kills the encoding process, so there is no image which could be decoded. Disabling assertions would work around that, but since charls would be trying to shift by negative amounts, I don't think that the result could be a valid JPEG-LS bitstream.

We seem to be using a CharLS version based on a snapshot from 2010-10-05, svn revision 55020. I tried to send all our important local patches upstream (but there are some minor ones which integrate into our plattform abstraction which you wouldn't be interested in) and I tried to apply all important patches from CharLS into our local tree.
Of couse this isn't quite a good state, so a new release would be a good excuse for me to clean that mess up. :-)

MarcoNeoL wrote May 14, 2012 at 8:27 AM

Hi Jan,

with reference to your sentence:
,
I think this is actually the same test case!
In fact, we reported the issue both to you and on the DCMTK forum.

What we do not know (and DCMTK developers certainly know better) is if/where the CharLS embedded into DCMTK is different from the "standard" CharLS.

If we can help further somehow, please let us know.
Thanks,

Marco

jdv wrote May 14, 2012 at 2:14 PM

re decodability: When compiling with Visual Studio, all ASSERT()s are removed in release mode.

I thought it also worked that way for the cmake based build. Will check that later.

MarcoNeoL wrote May 14, 2012 at 3:00 PM

Hi.

as you certainly know, ASSERTs are disabled in Visual Studio when the "NDEBUG" symbol is defined, which is normally the case for "Release" builds.

Nevertheless, in our specific case we built OFFIS DCMTK (embedding CharLS) starting from OFFIS v3.6.0 CMake files, and for some reason our "Release" build of OFFIS DCMTK did NOT have the "NDEBUG" symbol defined. Not sure if this depended on some "peculiarities" of the makefiles...

Hence, we were having ASSERTs (which related aborts) also in Release mode.

Best regards,

Marco

CodingNickNick wrote Jun 13, 2014 at 9:23 AM

Is this fixed? I'd check it myself but cloning fails:
nick@pc:~/src$ git clone https://git01.codeplex.com/charls charls-upstream                      
Cloning into 'charls-upstream'...
remote: Counting objects: 1936, done.
remote: Compressing objects: 100% (483/483), done.
remote: Total 1936 (delta 1364), reused 1823 (delta 1288)
error: RPC failed; result=56, HTTP code = 200 | 998.00 KiB/s   
Receiving objects: 100% (1936/1936), 7.63 MiB | 1.04 MiB/s, done.
Resolving deltas: 100% (1364/1364), done.
✗ - status code 128 

vbaderks wrote Jun 15, 2014 at 10:29 AM

Hi,

I am not sure if this is fixed. I currently don't have access to image that causes this problem. I am in the process of setting up a unit test that could verify if this problem exists or not, but that work has not been completed yet.

I would be great if you could attach the image that causes this issue to this thread. Creating a test with the image is much easier.

Feedback on git cloning problem
When I tried to clone CharLS to my Ubuntu test VM I got also some problems with cloning the repository. This seems to be causes by the git server used by codeplex and some git client implementations. You may need to checkout this solution: https://codeplex.codeplex.com/workitem/26133

Victor

awik wrote Jun 17, 2014 at 7:22 PM

It seems that there is still the issue in EncoderStrategy (v.1.0). You can find attached a test unit showing that for some combination of input data the process of encoding and decoding doesn't retain the initial data.
When 'if (bitpos >= 0)' statement is replaced with 'if (bitpos > 0)' then everything is fine.
Disabling ASSERT in Release mode only masks the issue.

CodingNickNick wrote Jun 24, 2014 at 12:39 PM

awik: I don't see the attachment, not sure if that's only me or not.

Are you saying we're losing data if this assertion would trigger, and that the correct fix is to change that statement you mentioned? That's easy to do for us as well, at least we'll have a nice lossless compression then.

Thanks in advance,

awik wrote Jun 24, 2014 at 3:46 PM

Link to the attachment with test case:
https://www.codeplex.com/Download/AttachmentDownload.ashx?ProjectName=charls&WorkItemId=10742&FileAttachmentId=865457

And yes, it can observed in this test case that when you only mask ASSERT statement the output data stream is incorrect. If you apply the fix then it is OK.

vbaderks wrote Jun 26, 2014 at 9:14 AM

Thanks for the attachment. I will try to merge this fix into the code.

vbaderks wrote Jan 10, 2015 at 6:35 PM

Fixed in changeset c7cf959f348f8e0c47f1197c89ef959372c572e9

vbaderks wrote May 27, 2016 at 11:45 PM

This has been (finally) fixed in https://github.com/team-charls/charls/issues/9