feat(parser): support transfer syntax inference when parsing small DICOM fragments (<100 bytes)

[mlibanori]

This PR introduces the SkipTransferSyntaxDetection option in the DICOM parser. The primary motivation for this change is to enable compatibility between this project and go-netdicom, which requires parsing DICOM network messages without enforcing transfer syntax detection.

[suyashkumar]

Thanks for the contribution! Out of curiosity, is there some reason why the auto-detection doesn't work?

dicom/parse.go

Lines 179 to 180 in 0fbaef5

	// No transfer syntax found, so let's try to infer the transfer syntax by
	// trying to read the next element under various transfer syntaxes.

Or is the idea to make this behavior more explicit?

[mlibanori]

Thanks for the review!

The main reason for this change is that certain DICOM commands, such as C-ECHO, can be smaller than 100 bytes. When this happens, the auto-detection logic fails with the error:

dicom/parse.go

Line 184 in 0fbaef5

return nil, fmt.Errorf("dicom with missing transfer syntax metadata is shorter than 100 bytes, so cannot infer transfer syntax")

Another possible fix would be to reduce p.reader.rawReader.Peek(100) to p.reader.rawReader.Peek(50), which would allow reading smaller PDU messages.

dicom/parse.go

Line 181 in 0fbaef5

next100, err := p.reader.rawReader.Peek(100)

However, since I'm not entirely sure why the current value is set to 100, I opted to add an option to bypass the transfer syntax detection restriction rather than directly changing the default behavior.

[suyashkumar]

Makes sense! First off, I think adding an option if needed is totally reasonable. Out of curiosity, I'd like to see if we can make it work without an option (if possible and not too complex).

There's no real reason for the 100 byte explicit limit, other than the thought that at least one element would fit in there and that most dicoms would be at least 100bytes. Both assumptions may not hold true, as you point out in this case!

We can probably make this work for dicoms < 100 bytes:

• Attempt to read 100 bytes, and if that results in io.EOF:
• Read as much as possible until io.EOF exhaustion, and continue forward with the existing auto-detection. (We can use io.ReadAll for this).
• We should also correctly handle non io.EOF errors here:

  dicom/parse.go

  Line 182 in 0fbaef5

  if errors.Is(err, io.EOF) {

It may not read as nicely, but we could also try to use io.ReadFull for 100 bytes, and simply intentionally ignore a io.ErrUnexpectedEOF to allow smaller reads. We could also wrap this in "readAtMost(n int)" helper which might make it more readable.

What do you think? If you're willing to help with this change, please go ahead! We'll likely want some tests to cover this case as well.

(Also, I'm curious if your payloads pass the autodetection test)!

[mlibanori]

Thank you for the review! I really appreciate the suggestion. Based on your feedback, I implemented the PeekAtMost(n) function, which ignores EOF errors and returns the maximum available buffer size.

dicom/pkg/dicomio/reader.go

Lines 238 to 244 in 0c00440

	func (r *Reader) PeekAtMost(n int) ([]byte, error) {
	peeked, err := r.in.Peek(n)
	if err == io.EOF || err == io.ErrUnexpectedEOF {
	return peeked, nil
	}
	return peeked, err
	}

I also developed a test using a buffer captured from a real DICOM communication, and the parsing was successfully completed.

dicom/parse_test.go

Lines 78 to 106 in 0c00440

	func TestParse_CEchoRQ(t *testing.T) {
	commandBytes := []byte{
	0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00,
	0x12, 0x00, 0x00, 0x00, 0x31, 0x2E, 0x32, 0x2E, 0x38, 0x34, 0x30, 0x2E, 0x31, 0x30, 0x30, 0x30,
	0x38, 0x2E, 0x31, 0x2E, 0x31, 0x00, 0x00, 0x00, 0x00, 0x01, 0x02, 0x00, 0x00, 0x00, 0x30, 0x00,
	0x00, 0x00, 0x10, 0x01, 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x08, 0x02, 0x00,
	0x00, 0x00, 0x01, 0x01,
	}
	ioReader := bytes.NewReader(commandBytes)
	dataset, err := dicom.Parse(ioReader, int64(len(commandBytes)), nil, dicom.SkipPixelData(), dicom.SkipMetadataReadOnNewParserInit())
	if err != nil {
	t.Fatalf("unexpected error: %v", err)
	}
	tags := []tag.Tag{
	{Group: 0x0000, Element: 0x0000},
	{Group: 0x0000, Element: 0x0002},
	{Group: 0x0000, Element: 0x0100},
	{Group: 0x0000, Element: 0x0110},
	{Group: 0x0000, Element: 0x0800},
	}
	for _, tag := range tags {
	_, err := dataset.FindElementByTag(tag)
	if err != nil {
	t.Fatalf("unexpected error: %v", err)
	}
	}
	}

If you have any concerns or think there’s a better approach, I’m happy to iterate further

PS: I noticed that this project does not include the Command Set tags (group 0x0000). Is there a specific reason for this, or have they simply not been implemented yet?

If you’re open to it, I’d be happy to submit a new pull request contributing to the generation of these tags as well.

[suyashkumar]

Thanks! Overall looking on right track, just a few more comments!

Regarding Command Set tags, you're absolutely welcome to help get those added. We're using the innolitics json dump of the standard to do this, so you're welcome to look into where those exist in there (if at all): https://github.com/suyashkumar/dicom/blob/main/pkg/tag/generate_tag_definitions.py . Thanks for the contributions, and always open to contributions as always!

[mlibanori]

@suyashkumar, do you have any other revisions?

[suyashkumar]

Thanks! Apologies for the delay. This is looking good, just added a few more suggestions below and one question. I think we're pretty much there!

[mlibanori]

all done! lets go?

[mlibanori]

@suyashkumar Are any further changes needed? Can we proceed?