Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow writing XMP to JPEG files #65

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

michaelosthege
Copy link

Adds a Root property to XmpDirectory such that the XDocument can be accessed directly.
Load behavior is modified to set the Root property.
Writing is accomplished by substituting existing Xml in App1 segments with new Xml, or inserting it if not present in the first place.

See #23 for more information.

@drewnoakes
Copy link
Owner

Just wanted to say that I'm pumped to get this feature into the library, but I want to give it a proper review because it'll be the first time the library is writing data and therefore the first time the library could potentially destroy users' files. I'm really busy at the moment but hope to find some time for this soon. Thanks for your patience.

@drewnoakes drewnoakes changed the title added XmpDirectory.Root and classes for writing Xmp to Jpeg files Allow writing XMP to JPEG files Nov 7, 2016
@drewnoakes
Copy link
Owner

Quick update. I've reworked the JPEG reading code to be more robust. It will allow better performance and safety for injecting/replacing XMP in JPEG files. Haven't pushed it yet as I want to do some more tidying and it's late here now.

@michaelosthege
Copy link
Author

I just updated my branch with your latest changes & also some refactorings.

  • refactored it such that the algorithm is easier to understand, review & test,
  • Unit tests for the Jpeg Xmp writing are still ToDo - any suggestions for a good test case?

I had used http://ns.adobe.com/xap/1.0/\x0 as a segment preamble before and now I use XmpReder.JpegSegmentPreamble wich equals http://ns.adobe.com/xap/1.0/\0 (without the x). (My reference was this)

In both reading & writing, could the differences in the end of the preamble lead to segments that are not recognized as XMP segments? (And thereby ignoring them in read, or adding a duplicate segment in write?)

@michaelosthege
Copy link
Author

I finished a lot of refactoring to accomplish a few things:

  • metadata writing is abstracted in the same way as reading
  • the new design simplifies the addition of support for writing other types of metadata
  • writing to Jpeg files is implemented such that there is no impact on other parts of the file.

For writing to a file, the original data is fragmented into a list of JpegFragment objects. In contrast to JpegSegment, the JpegFragment.Bytes also includes the segment marker and size bytes.
By concatenating all JpegFragments, one gets back the original file. To write metadata, an IJpegMetadataWriter such as the XmpWriter just iterates over the fragments to find an existing JpegSegment with metadata and replaces it with the updated data. If none is found, the XmpWriter inserts a new JpegFragment of an App1 Xmp segment into the collection.

Finally I added a bunch of tests (& test data) that verify the correct behavior at multiple abstraction levels.

  • testing the splitting & concatenation of Jpeg data into fragments
  • testing the Xmp specification compliant encoding of an Xmp XDocument into a JpegFragment (App1 marker, size bytes, serialized Xmp payload)
  • testing the insertion/replacement of Xmp metadata into a collection of JpegFragments
  • testing the insertion/replacement of metadata at the level of the Stream

With these modifications, Xmp metadata can be written (inserted/replaced) into a Stream of Jpeg data like this:

var metadata_objects = new object[] { xmpXdoc };
var updatedStream = ImageMetadataWriter.WriteMetadata(originalStream, metadata_objects);

@elangelo
Copy link

@drewnoakes any chance this can be picked up again? i'm willing to merge the conflicts and to follow up on any remarks you still have after review.

@drewnoakes
Copy link
Owner

@elangelo the barriers to merging this are 1) my free time to review and really think deeply about this major change to the library and 2) my reluctance to take on the risk and responsibility involved with writing metadata to files and potentially breaking people's data. The cost of bugs becomes much higher once we start writing data. That said, there's regular interest in doing so. If you're interested in helping with this PR, then reviewing the general approach and thinking through how other kinds of metadata may be written would be helpful. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants