search

neilharvey / FileSignatures

A small library for detecting the type of a file based on header signature (also known as magic number).
MIT License
250 stars 41 forks source link

Add WEBP && MP4 formats support #45

Closed hell03end closed 2 years ago

hell03end commented 2 years ago

Add support for 2 new formats:

Aslo minor refactoring in related code

hell03end commented 2 years ago

I can undo refactoring changes if necessary.

neilharvey commented 2 years ago

Hey, thanks for the PR!

For the new formats, instead of having static fields on the new Mp4 and the Webp classes could you instead create these as subclasses instead? i.e. have the base Mp4 class handle the common signature part, then have MP4v1, M4v, Mov etc. as subclasses of Mp4 that handle their particular format. Similarly, reading the Google documentation it looks like Riff is the container format for Webp so would be the base class.

For an existing example, have a look at the JPEG format which has subtypes of EXIF, JFIF and SPIFF.

The reason I prefer this model is that it offers more flexibility - for example if we want to add a new format which uses the Mp4 or Riff container then we can just add it as a subclass. Or if we are sure we are not interested in certain types of MP4 then those can be excluded from the registered formats.

The refactoring changes in 0dca7cb86f1e0154e1e3b218a4e74864c5e88a74 are fine, no problems with those. Similar with the new Video abstraction in 14636937f0dd68da610baeb201b31e18df172a32 that looks good too.

neilharvey commented 2 years ago

Also - do you have any small samples which can be used to test the formats work as expected? The test project contains a directory of Samples which can be plugged into the test here to verify that they are detected.

hell03end commented 2 years ago

Hi, thanks for comments!

I added separate classes for each of the FTYP-based formats (mostly mp4-based), but it was difficult to separate the RIFF container from the WEBP format and leave it as an Image subclass (because several base classes are not supported in C#). I also added some samples and tests for them and did some refactoring of the tests to fix the warnings that my IDE is giving me. Hope it is ok.

neilharvey commented 2 years ago

Hey, thanks for the changes - I will have a look through and see about merging this in.

One thing I would recommend for any future pull requests - keep it simple! There is an awful lot to look at here, and mixing refactoring with new features makes it hard to pick apart the changeset. :)

Cheers!

neilharvey commented 2 years ago

Merged into new_formats_webp_mp4.

neilharvey commented 2 years ago

Closing as merged.