Values with VRs constructed of character strings are padded with trailing NULL instead of spaces

[selimyoussry]

Per http://dicom.nema.org/dicom/2013/output/chtml/part05/sect_6.2.html, paragraph 6.2 Value Representation (VR):

Values with VRs constructed of character strings, except in the case of the VR UI, shall be padded with SPACE characters (20H, in the Default Character Repertoire) when necessary to achieve even length. Values with a VR of UI shall be padded with a single trailing NULL (00H) character when necessary to achieve even length. Values with a VR of OB shall be padded with a single trailing NULL byte value (00H) when necessary to achieve even length.

When parsing a DICOM file with RequestAttributesSequence (among other tags encoutering the same issue), with pydicom, the trailing NULL byte is not removed by pydicom, which is probably following the DICOM spec referenced above more closely
dcm_file = pydicom.dcmread(...) print(dcm_file.RequestAttributesSequence[0]) Out[33]: (0040, 0007) Scheduled Procedure Step Descriptio LO: 'G2702\x00' (0040, 0009) Scheduled Procedure Step ID SH: 'G2702\x00' (0040, 1001) Requested Procedure ID SH: '918081'

For the record, dcmdump does not have the same issue and removes trailing NULL bytes.

That being said, we are using your Go DICOM library to write some DICOM files, which are later consumed by Python users, with pydicom. I believe for "VRs constructed of character strings, except in the case of the VR UI", which would be "DT", "LO", "LT", "PN", "SH", "ST", "UT" per my read of http://dicom.nema.org/dicom/2013/output/chtml/part05/sect_6.2.html, that odd number of bytes should be padded by an empty space instead.

I patched your code with this change and can confirm that pydicom then parses the values as expected, so does your library and so does dcmdump. See output below

print(dcm_file.RequestAttributesSequence[0])
Out[42]: 
(0040, 0007) Scheduled Procedure Step Descriptio LO: 'G2702'
(0040, 0009) Scheduled Procedure Step ID         SH: 'G2702'
(0040, 1001) Requested Procedure ID              SH: '918081'

Here is the diff

git diff
diff --git a/write/writer.go b/write/writer.go
index d4fe85a..d04ad02 100644
--- a/write/writer.go
+++ b/write/writer.go
@@ -384,7 +384,14 @@ func Element(e *dicomio.Encoder, elem *element.Element, opts ...Option) {
                        }
                        sube.WriteString(s)
                        if len(s)%2 == 1 {
-                               sube.WriteByte(0)
+                               switch vr {
+                               // Values with VRs constructed of character strings, except in the case of the VR UI, shall be padded with SPACE characters
+                               // per http://dicom.nema.org/dicom/2013/output/chtml/part05/sect_6.2.html
+                               case "DT", "LO", "LT", "PN", "SH", "ST", "UT":
+                                       sube.WriteString(" ")
+                               default:
+                                       sube.WriteByte(0)
+                               }
                        }
                }
                if sube.Error() != nil {

Here is the make (and therefore test) output

 make
go mod download
/Library/Developer/CommandLineTools/usr/bin/make test
go test ./...
ok  	github.com/suyashkumar/dicom	1.020s
?   	github.com/suyashkumar/dicom/cmd/dicomutil	[no test files]
?   	github.com/suyashkumar/dicom/constants	[no test files]
ok  	github.com/suyashkumar/dicom/dicomio	0.027s
?   	github.com/suyashkumar/dicom/dicomlog	[no test files]
ok  	github.com/suyashkumar/dicom/dicomtag	0.015s
ok  	github.com/suyashkumar/dicom/dicomuid	0.028s
?   	github.com/suyashkumar/dicom/element	[no test files]
?   	github.com/suyashkumar/dicom/frame	[no test files]
ok  	github.com/suyashkumar/dicom/query	0.024s
ok  	github.com/suyashkumar/dicom/write	0.016s
go build -o build/dicomutil ./cmd/dicomutil

Can you please share guidelines to submit a PR for this? Or please feel free to simply open the PR yourself if you'd prefer not opening this repo to other contributors, and let me know. Happy to become a contributor though!

Best regards,

[suyashkumar]

Hi @selimyoussry, thank you for the detailed context. I think this proposed change makes sense to me, please feel free to submit a pull request to this repo! Contributions are very welcome.

As for guidelines, I don't have any special requirements--I guess the main thing to keep in mind is that submitting a PR to this repo means you are okay licensing your patch under this repository's license (the standard MIT license). This is generally true of all of GitHub.

If you're interested in contributing more to this repo in general, let me know and I'm happy to keep you in mind for future issues or feature development (I have a rewrite in progress at the s/1.0-rewrite branch, that cleans things up significantly and refactors the library a bit).

[suyashkumar]

Also, for VRs where leading and trailing spaces are considered insignificant, it may be worth trimming those by default when parsing as well--feel free to send a contribution for that as well, otherwise I'll open a separate issue to track that as well.

Thank you!

[selimyoussry]

Also @suyashkumar , when you review my PR and it gets accepted and merged, can you please take care of the go.mod version bump? Thanks

[suyashkumar]

closed by #81

[suyashkumar]

@selimyoussry apologies for the delay, but you should be able to use the v0.4.3 tag: https://github.com/suyashkumar/dicom/releases/tag/v0.4.3