properly strip trailing null characters (\0) from tag data

[gtiwari333]

A change from https://github.com/drewnoakes/metadata-extractor/pull/634/files#diff-7df436f0258b5156fd15792db94b4cab6119180ad7d605533ffb53213f048ba2R400 got missed in earlier merge.

#635

[gtiwari333]

This PR will fix the extraction of picture control name that's coming up with trailing 0s.

[drewnoakes]

Thanks for following up here. I intentionally changed your last PR before merging to avoid the approach here though. Let's see if there's a different approach that'll work for you.

When pulling strings from bytes, we generally store StringValue rather than String. This allows the consumer to reinterpret the underlying data in different encodings. If we convert to string, that data is lost.

The problem you're seeing is that StringValue is not trimming trailing null bytes. I wonder if there's a better way to deal with that.

[drewnoakes]

I understand now. I introduced a bug in your PR — sorry about that! I should have waited for your input before merging, but you said you wanted a release so I pushed ahead.

The values read from this format are null terminated and should be trimmed. There's already a method we can use in that case! Instead of adding getNullTerminatedStringAndSkipToNextPosition, we can call getNullTerminatedStringValue . That will simplify the fix here a bit, and it'll keep the StringValue so the value can be re-encoded by the consumer if needed.

I'll get another release out with your fix when it's ready.

Thanks!

[drewnoakes]

We don't need to add a new method. We can call getNullTerminatedStringValue instead.

[drewnoakes]

In order to preserve the StringValue this can be:

Suggested change

	directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getNullTerminatedStringAndSkipToNextPosition(4, Charsets.UTF_8).toString());
	directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString());
	directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString());
	directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getNullTerminatedStringValue(4, Charsets.UTF_8));
	directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getNullTerminatedStringValue(20, Charsets.UTF_8));
	directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getNullTerminatedStringValue(20, Charsets.UTF_8));

[drewnoakes]

Ditto:

Suggested change

	directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getNullTerminatedStringAndSkipToNextPosition(4, Charsets.UTF_8).toString());
	directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString());
	directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getNullTerminatedStringAndSkipToNextPosition(20, Charsets.UTF_8).toString());
	directory.setString(TAG_PICTURE_CONTROL_VERSION, reader.getNullTerminatedStringValue(4, Charsets.UTF_8));
	directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getNullTerminatedStringValue(20, Charsets.UTF_8));
	directory.setString(TAG_PICTURE_CONTROL_BASE, reader.getNullTerminatedStringValue(20, Charsets.UTF_8));

[drewnoakes]

Please avoid reformatting code in PRs that also include functional changes. Formatting changes are appreciated when they change code towards the predominant style in the repo. We don't need the getNullTerminatedStringAndSkipToNextPosition method, nor its unit test. The toString calls below don't seem to do anything, and the formatting changes are unrelated to the PR. I think this file can be exluded from the PR altogether.

[drewnoakes]

I ported the fix to .NET as well here: drewnoakes/metadata-extractor-dotnet#349

[drewnoakes]

And CI is fixed again. Sorry about that.

[gtiwari333]

Instead of adding getNullTerminatedStringAndSkipToNextPosition, we can call getNullTerminatedStringValue

There's a problem with getNullTerminatedStringValue. After reading the requested number of bytes from input, it doesn't move the cursor to the end. My test testGetNullTerminatedStringCursorPositionTest explains what's wrong with getNullTerminatedStringValue method. Only way to get around it is to check the size of text returned and manually skip the remaining bytes.

For input AB\0\0\0CD\0ABCDEF

If you ask for getNullTerminatedString(5) it will return AB but keep the cursor position at index 3.

The new method getNullTerminatedStringAndSkipToNextPosition reads the AB and move the cursor at C.

If you compare testGetNullTerminatedStringCursorPositionTest and testGetNullTerminatedStringAndSkipToNextPosition you will see the improvement the new method makes.

[gtiwari333]

Did few more testing and found an easy way to deal with it. Let me know what would you prefer?

The getStringValue reads the all 20 bytes and moves the cursor to the end. If we trim the value using String.trim(), it will remove the trailing null characters.

directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getStringValue(20, Charsets.UTF_8).toString())

Change this to:

directory.setString(TAG_PICTURE_CONTROL_NAME, reader.getStringValue(20, Charsets.UTF_8).toString().trim())

[drewnoakes]

I ended up fixing this in #647 with an approach that mirrors what we did in the .NET version, so that they're as close as possible (which helps porting other changes in future). It also avoids adding toString calls to reduce allocations and allow consumers to reinterpret the StringValue in other encodings if needed.

[drewnoakes]

Oops. Thanks!

[Nadahar]

@drewnoakes I deleted my comment because I assumed you would edit yours - but ended up making it make no sense at all 😊