Port of Java #231: Implement Huffman Tables directory and reader #197
Conversation
There was a problem hiding this comment.
Thanks for porting this! Some comments inline.
Some of these comments are ways the C# implementation could be improved over the Java one. Others are possibly improvements that should be made in future to the Java library as well. For the latter case, let's open an issue in the Java repo to track them.
…e method to fill it instead - change HuffmanTable to readonly struct - use Linq ToArray() instead of manual byte copying for _lengthBytes and _valueBytes - add JpegDnlDirectory and JpegDnlDescriptor - add ByteArrayUtil.EqualTo for correct byte array comparison - do JpegDnl optional checking in JpegMetadataReader instead of entering JpegDnlReader - JpegDhtReader extraction should produce only one HuffmanTablesDirectory regardless of segment count - throw MetadataException in JpegDhtReader.GetBytes when stuffing is invalid
|
@drewnoakes I think most of these issues have been addressed. Please review it again and let me know if there are other questions or concerns. |
| /// <returns>The List of HuffmanTables in this Directory.</returns> | ||
| [NotNull] | ||
| public List<HuffmanTable> Tables { get; set; } = new List<HuffmanTable>(4); | ||
| private List<HuffmanTable> Tables { get; set; } = new List<HuffmanTable>(4); |
There was a problem hiding this comment.
I'd prefer a field here over a private property. It'd be more consistent with the rest of the code.
| // This Extract structure is a little different since we only want to return one HuffmanTablesDirectory | ||
| // for one-to-many segments | ||
| var directories = new List<Directory>(); | ||
| if (segments.Count() > 0) |
There was a problem hiding this comment.
This method ends up enumerating segments twice. It'd be good to do it only a single time. Perhaps declare directory as null initially, then allocate lazily within the foreach loop and only add it after the loop if it's non-null.
| } | ||
|
|
||
| return new List<Directory> { directory }; | ||
| return directories; |
There was a problem hiding this comment.
It should be possible to avoid allocating a List<> in cases where no DNL segment exists.
For example:
class Directory
{
internal static readonly DirectoryList EmptyList = new Directory[0];
}Then here if directory is null just return Directory.EmptyList.
Note that in the general case we'd use Array.Empty<Directory>() but because we also target net35 that's not an option.
BTW this is a minor thing so don't worry if you don't think it's worth it.
There was a problem hiding this comment.
This does "work" but not cleanly. DirectoryList needs to also be defined in the Directory class but 'using' can't be inherited so all the other DirectoryList preprocessor conditionals still must exist.
I agree this is relatively minor so I'll have it return an empty list for now. If something changes in the future to make this easier, it can be revisited.
There was a problem hiding this comment.
DirectoryList is just a local alias, so you'd only need to copy it into the Directory class, not all its consumers too.
- remove some "Ported from Java..." license comments - "JpegDnl" -> "JPEG DNL" for descriptor name - change HuffmanTablesDirectory.Tables property to a field (_tables)_tables - avoid enumerating segments twice in JpegDhtReader and lazy load instead - make ByteArrayUtil.EqualTo "Pure" - made some internal variables in HuffmanTablesDirectory public for unit testing purposes
|
More refinements as requested. Let me know how it looks. |
- Add EmptyList static variable to Directory class to use when a reader doesn't return anything
|
Round 4 is ready. |
Port of drewnoakes/metadata-extractor#231