Update JpegSegmentPreamble to UTF-8 spans #376
Conversation
There was a problem hiding this comment.
Just some questions before merge.
The AdobeJpegReader compares the Preamble using a case-insensitive comparer in ReadJpegSegments, while it uses an ordinal comparer when reading segments. From what I can tell in the specifications, this should be exact, but I didn't want to change behavior here.
That sounds like a bug. My guess is it should be case-insensitive everywhere. Can run against the regression suite to see if we have any hits.
| @@ -1,7 +1,7 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
| <TargetFramework>net45</TargetFramework> | |||
| <TargetFramework>net48</TargetFramework> | |||
| @@ -90,7 +90,7 @@ public void Add(T value, params byte[][] parts) | |||
| } | |||
|
|
|||
| /// <summary>Store the given value at the specified path.</summary> | |||
| internal void Add(T value, ReadOnlySpan<byte> part) | |||
| public void Add(T value, ReadOnlySpan<byte> part) | |||
There was a problem hiding this comment.
Is the byte[] overload still used?
Does this need to be added to the public API files?
There was a problem hiding this comment.
This was a brand new member to avoid allocation, the params byte[] array overload still used.
There was a problem hiding this comment.
Is there a diagnostic here about the public member not being in the API files?
There was a problem hiding this comment.
Annoying, this analyzer randomly stops reporting for me in Visual Studio, and I have to catch these manually. Updated!
There was a problem hiding this comment.
Weird. There should at least be a build warning, and CI should fail. I'll take a look some time.
There was a problem hiding this comment.
The Adobe case is a shame. It's the only one that doesn't fit. What do you think about changing the interface to include a method that validates whether a given segments bytes are a match or not? Then the Adobe one could implement it's policy within that method. The dictionary in the PowerShell code would be swapped for iteration through known extractors, testing for a match.
|
|
||
| private static ReadOnlySpan<byte> JpegSegmentPreambleBytes => "http://ns.adobe.com/xap/1.0/\0"u8; | ||
| private static ReadOnlySpan<byte> JpegSegmentPreambleExtensionBytes => "http://ns.adobe.com/xmp/extension/\0"u8; | ||
| public static ReadOnlySpan<byte> JpegSegmentPreamble=> "http://ns.adobe.com/xap/1.0/\0"u8; |
There was a problem hiding this comment.
| public static ReadOnlySpan<byte> JpegSegmentPreamble=> "http://ns.adobe.com/xap/1.0/\0"u8; | |
| public static ReadOnlySpan<byte> JpegSegmentPreamble => "http://ns.adobe.com/xap/1.0/\0"u8; |
| @@ -90,7 +90,7 @@ public void Add(T value, params byte[][] parts) | |||
| } | |||
|
|
|||
| /// <summary>Store the given value at the specified path.</summary> | |||
| internal void Add(T value, ReadOnlySpan<byte> part) | |||
| public void Add(T value, ReadOnlySpan<byte> part) | |||
There was a problem hiding this comment.
Is there a diagnostic here about the public member not being in the API files?
@drewnoakes Ready for review / feedback.
Note: The AdobeJpegReader compares the Preamble using a case-insensitive comparer in ReadJpegSegments, while it uses an ordinal comparer when reading segments. From what I can tell in the specifications, this should be exact, but I didn't want to change behavior here.