feat: Make format optional for Reader to allow native lib to guess format - #251
Conversation
…eader-type' into mathern/guess-reader-type
gpeacock
left a comment
gpeacock
left a comment
There was a problem hiding this comment.
That's a LOT of tests added. Was this all driven by the format detection stuff, or were we generally missing tests here? This seems like a lot to support at one binding level vs driving the logic and tests to the rust layer.
Yes, all the formats variations with blank strings, padding and the like. I also verified the behavior for builder and the embeddable APIs (making sure there are not surprises). Update: I also ported some tests from Python that caused surprising errors there (looking at you, DNG). I have many ideas how to try to break this, so I added matching coverage... I'm not against pulling things down in Rust, but that means also when we do C++ only, we're more blind/trusting of the Rust coverage (I do not necessarily run the Rust tests when I do C++ only). It makes me feel better to have the test coverage here too. |
ale-adobe
left a comment
ale-adobe
left a comment
There was a problem hiding this comment.
I know already merged, but just had some comments that I didn't get to submit beforehand.
| { | ||
| private: | ||
| C2paReader *c2pa_reader; | ||
| C2paReader *c2pa_reader = nullptr; |
| /// different one wins over @p format, and if nothing is detected @p format is | ||
| /// used as given. Pass an empty string to rely on detection alone. A format | ||
| /// absent from supported_mime_types() is not rejected here. | ||
| /// @param stream The input stream to read from. Must support seeking; it is rewound |
There was a problem hiding this comment.
Do we enforce the seeking requirement in any way, or do we just end up erroring and say it's the user's fault? If we throw an error, I think the comment below needs updating.
| /// overload taking a format when the format is known. | ||
| /// The stream is rewound before inspection, so it must support seeking. | ||
| /// @param context Shared context provider. | ||
| /// @param stream The input stream to read from. Must support seeking. |
There was a problem hiding this comment.
nit, for consistency. Also noticed this a few more times below.
| /// @param stream The input stream to read from. Must support seeking. | |
| /// @param stream The input stream to read from. Must support seeking; it is rewound before inspection. |
|
|
||
| /// Formats that count as absent: empty, or only ASCII whitespace. | ||
| inline const std::vector<std::string> kBlankFormats = { | ||
| "", " ", " ", "\t", "\n", "\t\n ", "\r\n", "\v\f"}; |
There was a problem hiding this comment.
Is the third string here three spaces? Why does that need its own entry?
3 participants
Make format optional for Reader to allow native lib to guess format (+overloads for the Reader).