Add Support for Quicktime metadata incl GPS and CreationDate #241
Conversation
There was a problem hiding this comment.
Looks great. Capturing a list is a much nicer solution than passing a data object around.
Two comments about preventing exception. In general the library works to only raise exceptions due to IO issues (end of stream/permission error/etc), however malformed file data should not throw but instead add errors to dictionaries. This generally allows more metadata to be read.
| var typeIndicator = a.Reader.GetBytes(4); | ||
| var localeIndicator = a.Reader.GetBytes(4); | ||
|
|
||
| // Data Atom | ||
| var dataTypeIndicator = a.Reader.GetBytes(4); | ||
| var dataLocaleIndicator = a.Reader.GetBytes(4); |
There was a problem hiding this comment.
Looking at the spec, it seems that these values cannot be ignored. The value is not guaranteed to be a UTF-8 string. The type indicator is packed to contain a well-known type field which instructs the encoding to use for the data value (e.g. UTF-8, UTF-16, etc...).
Consider reading int32 values rather than byte[] as the former will not cause heap allocations.
We don't have any built-in support for per-value locale support. It may be interesting to add, though this would be the only usage I know of currently.
There was a problem hiding this comment.
I took some of my assumptions from Table 3-6 (though this would leave Artwork and UserRating improperly handled).
Are you looking for a full implementation of Well-Known Types or would you be okay with providing support for types 1, 13, 14, 23 and 27 and treating other values as errors?
As for locale, I was hoping to provide an incremental improvement by adding support for GPS location retrieval, All of the samples that I had access to had this value set to \0\0 so I did not have a need for it at the time.
There was a problem hiding this comment.
would you be okay with providing support for types 1, 13, 14, 23 and 27 and treating other values as errors?
Yes that's a long table and I don't think we need to handle everything. Adding the framework to support different encodings and covering the ones we think would be most common and easiest is great for now. Having even basic support now lowers the bar for others who stumble upon data that's not covered.
I would avoid treating unsupported encodings as errors. Maybe just store the raw bytes instead. That's more in line with what's done elsewhere.
| var dataTypeIndicator = a.Reader.GetBytes(4); | ||
| var dataLocaleIndicator = a.Reader.GetBytes(4); | ||
| var data = a.Reader.GetBytes((int)atomSize - 24); | ||
| var dataStr = Encoding.UTF8.GetString(data); |
There was a problem hiding this comment.
This should use StringValue with the encoding defined by the type indicator. This allows a consumer to choose a different encoding at some later point. We regularly see data where the advertised encoding is incorrect, and StringValue gives users a way forward.
|
Thanks for this and apologies it took a while to get to it. I pulled the code down and played with it a bit. Very nice! Would love to have more QuickTime files for testing against. If you have any you could share, please do so. |
|
Glad it worked out. I don't have any more examples that I can share atm, but if I find good candidates i will send them along. Thanks. Cheers. |
Does not support Optional Item_info atom or Optional name atom inside the the Value Atom as described in the spec as none of the sample data I had access to had these attributes.