Wrong frame returned by DicomPixelData when highly parallel

[tboby]

Wrong frame returned by DicomPixelData when highly parallel

[gofal]

The rendering never have been perfectly thread-safe. many values have been stored in local fields of the class. This become more critical with EnhancedCT or EnhancedMR where the rendering of frames are using different pipelines and therefore the pipielines are recalcualted depending on the last rendered frame, and then it happes that the right pixel data was loaded, but rendered with the wrong pipeline.
This is, why the rendering has been redesigned.

@tboby Would you be so kind and do the testing with the code in the following branch:
https://github.com/fo-dicom/fo-dicom/tree/GH917-RenderingOfEnhanced
This should be stable, even if the code is not yet optimized.

[tboby]

@tboby Would you be so kind and do the testing with the code in the following branch:
https://github.com/fo-dicom/fo-dicom/tree/GH917-RenderingOfEnhanced
This should be stable, even if the code is not yet optimized.

This doesn't change the failing test: I don't think it's really a problem with the rendering, more with accessing Data from StreamByteBuffer in parallel. The same test adjusted to avoid DicomPixelData completely:

[Fact]
        public void DirectOtherWord()
        {
            // Arrange
            var inputFile = new FileInfo(TestData.Resolve("GH645.dcm"));
            using var fs = File.OpenRead(inputFile.FullName);
            using var ms = new MemoryStream();

            fs.CopyTo(ms);

            ms.Seek(0, SeekOrigin.Begin);

            var streamByteDicomFile = DicomFile.Open(ms);

            var referencePixelData = streamByteDicomFile.Dataset.GetDicomItem<DicomOtherWord>(DicomTag.PixelData);
            var pixelData = DicomPixelData.Create(streamByteDicomFile.Dataset);
            var numberOfFrames = streamByteDicomFile.Dataset.GetSingleValue<int>(DicomTag.NumberOfFrames);
            var referenceBytes = Enumerable.Range(0, numberOfFrames).Select(x =>
                {
                    var offset = (long)pixelData.UncompressedFrameSize * x;
                    IByteBuffer buffer = new RangeByteBuffer(referencePixelData.Buffer, offset, pixelData.UncompressedFrameSize);
                    return buffer.Data;
                })
                .ToArray();

            // Act
            var streamBytePixelData = (streamByteDicomFile.Dataset.Clone()).GetDicomItem<DicomOtherWord>(DicomTag.PixelData);

            Parallel.For(0, 10000, i =>
            {
                var index = i % numberOfFrames;
                var offset = (long)pixelData.UncompressedFrameSize * index;
                IByteBuffer frame = new RangeByteBuffer(streamBytePixelData.Buffer, offset, pixelData.UncompressedFrameSize);
                Assert.Equal(referenceBytes[index], frame.Data);
            });
        }

I also ran our very small sanity check regression testset against that build: YBR_RCT and YBR_ICT failed, so I guess that temporary fix changing the photometric interpretation was doing something :)

[gofal]

Thanks, I see. The test only runs green (and slow) if the StreamByteBuffer gets a lock in all the Read-methods wher the stream position is set and then stream.Read is called.
But: with the new branch, where the actual pixeldata is cached within the DicomImage object instead of recreated on every RenderImage call, the likelyhood of this race condition is lowered dramatically.

so, to solve this, StreamByteBuffer and FileByteBuffer (the only two classes I found, where random access to an underlying stream by setting position and then reading from stream) require to have a lock mechanism.

[gofal]

The other thing, that interests me very much, is the YBR_RCT and YBR_ICT fail. I would really appreciate, if you could post some sample image to the discussion of the PR #1591 Because actually, my render tests did not show any regression, but rendered everything without issue.
Because the rendering was buggy until now, the rendering code in my application does some corrections and manipulations. With the new branch, you should throw that all away and just call DicomImage.RenderImage method and avoid that the "old workaround-code" now breaks the correct rendering.
Thanks!!

[tboby]

The test only runs green (and slow) if the StreamByteBuffer gets a lock in all the Read-methods wher the stream position is set and then stream.Read is called.

While this seems very slow, it shouldn't be that bad in reality as the decoding should dominate time compared to the stream reading.

where the actual pixeldata is cached within the DicomImage object instead of recreated on every RenderImage call, the likelyhood of this race condition is lowered dramatically.

We're only reading each frame once, and the memory usage of caching pixeldata would likely be a bigger pressure on resources than the cost of fast turbo-jpeg decoding. However, we also strip all overlay tags so our use-case is a little different. The caching seems great for an interactive viewer!

FileByteBuffer

The weird thing is that we don't experience this with FileByteBuffer: it was only when we swapped to sometimes accepting streams that it started happening. It looks like it might be creating a new FileStream per Read, which avoids the problem.

I would really appreciate, if you could post some sample image to the discussion of the PR #1591

Will do!

[gofal]

You're right, in real world all the other processing like decoding, rendering etc would be dominating the actual reading.
And with the caching, thats right. if you have some viewing applicaiton, where you call the rendering of the same frames often, then this is good for performance, for applications that only open a file, render each frame once and then are done, this caching is rather bad. This is why there is a property DicomImage.CacheMode where you can activate to cache everything, or nothing or something in between.

Your right, the FileByteBuffer creates a new reading stream on each access, did not see that yesterday when I searched for all setter-accessed to stream.position. So it's just about synclock access to StreamByteBuffer.

Thanks for pointing at this!