Is EvenLengthBuffer obsolete? It breaks multiframe SOPs with odd length frames when they are decompressed

[aerik]

Expected behavior

Decompress a 8 bit monochrome multiframe file with an odd number of pixels (my test was 499 * 709 which is 353791) and each frame is displayed correctly in commercial viewing software

Actual behavior

Each frame "drifts" right by one pixel

Steps to reproduce the behavior

Decompress a multiframe file with an odd number of pixels

fo-dicom version and OS/platform

4.0.4 Windows Desktop

[aerik]

An EvenLengthBuffer is use for the output pixels for each frame when decompressing multiframe images. In the rare case of 8 bit monochrome images with an odd number of pixels, this results in each frame "drifting" by one pixel.

EvenLengthBuffer seems to only be used in two places, in DicomOverlayDataFactory and inside Dicom.Imaging.Codec.Jpeg.i. This leads me to wonder if it is actually obsolete, since there must be some other mechanism to ensure that tag values are written with even lengths.

[aerik]

Proposed solution, in the absence of firm guidance, etc, is to remove the lines in jpeg.i that would force an even length.

if ((frameSize % 2) != 0) frameSize++;

and

buffer = EvenLengthBuffer::Create(buffer);

[gofal]

Did some code-reading meanwhile. The "kernel" of fo-dicom does not check for even length. obviously everyone inserting some data is responsible to insert correct data. This are either the developers who create new pixel data using fo-dicom, or the transcoders which create new pixeldata out of existing data. Thats why it is contained in jpeg codec.
I am little surprisec, because it should also be contained in RLE or similar..
Anyway: the transcoders do not handle whole frame stacks, but only single frames. So there is this missing gap...

[aerik]

I think we cannot say the transcoder is responsible for ensuring the pixel data element is an even length when it only handles single frames - is that what you are saying? FWIW, I made the changes I suggested and put them into production. We have not had any issues so far (though I did not confirm that we are not creating odd length pixel data elements - should probably do that)

[gofal]

Yes, thats what I wanted to say. The Transcoder only works on one frame. If the transcoder ensures that this frame is even, then the sum of all the frames must be even, too. But if the transcoder does not care any more, then there is currently no other class that cares if the sum of all the frames is even in the end.

I do believe that there is no problem in production (until now). All DICOM implementations I know try to be as tollerant as possible when reading data. Standard is clear with the even length. But an application that receives a file with odd length and says: I do not display this image because it is not standard, will soon get troubles with customers who say: But your competitor is able to display it... There are many things in DICOM (famous: UIDs must not have leading zeros, e.g. 1.2.004.5) obviously breaking standard but will never be any isue anywhere.

But still: fo-dicom should be as strict as possible when creating data, and should be as tollerant as possible when reading data.

[aerik]

Your math is of course correct (even length frame x whatever = even length pixel data) but the end result where every frame is padded by one pixel is unnecessary and I believe there is no mechanism for determining the actual starting stream position of frames after the first one (they will each be offset by one more byte than the previous frame). I would argue that having extra bytes interspersed in the pixel data is just as bad, if not worse, than an odd length element.

[gofal]

this PR #1410 could be a simple fix for the issue?

[mrbean-bremen]

the end result where every frame is padded by one pixel is unnecessary and I believe there is no mechanism for determining the actual starting stream position of frames after the first one (they will each be offset by one more byte than the previous frame)

Agreed. We had encountered such data (with a padding byte after each frame) and proposed a patch to dcmtk (which we used for reading the data), but it was declined, because it is not possible to definitely determine if this is the problem (and it is invalid DICOM, anyway). This is given that dcmtk does a lot of other stuff to read non-conformant DICOM.

this PR #1410 could be a simple fix for the issue?

Yes, I also think so. Note that there also had been a bug in the transcoder that caused this kind of incorrect padding, which only has been fixed this year.