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

image/jpeg: specify APP1 segment for outputting EXIF data in jpeg.Encode()? #12202

Open
jdeng opened this issue Aug 19, 2015 · 7 comments
Open

image/jpeg: specify APP1 segment for outputting EXIF data in jpeg.Encode()? #12202

jdeng opened this issue Aug 19, 2015 · 7 comments
Labels
Milestone

Comments

@jdeng
Copy link

@jdeng jdeng commented Aug 19, 2015

Currently there is no way to output EXIF in jpeg.Encode() so if a JPEG file is modified use image/jpeg the EXIF info is lost.

Reading EXIF using jpeg.Decode() has a similar problem but reopening the file and reading a few kilo bytes is probably an acceptable workaround. Reopening a file to insert a segment is much more cumbersome and inefficient.

One way to do this is to introduce a new field in jpeg.Options. Something like below diff. Thoughts?

diff --git a/src/image/jpeg/reader.go b/src/image/jpeg/reader.go
index adf97ab..5942609 100644
--- a/src/image/jpeg/reader.go
+++ b/src/image/jpeg/reader.go
@@ -64,6 +64,7 @@ const (
        // but in practice, their use is described at
        // http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/JPEG.html
        app0Marker  = 0xe0
+       app1Marker  = 0xe1
        app14Marker = 0xee
        app15Marker = 0xef
 )
diff --git a/src/image/jpeg/writer.go b/src/image/jpeg/writer.go
index 91bbde3..e404efa 100644
--- a/src/image/jpeg/writer.go
+++ b/src/image/jpeg/writer.go
@@ -541,6 +541,7 @@ const DefaultQuality = 75
 // Quality ranges from 1 to 100 inclusive, higher is better.
 type Options struct {
        Quality int
+       App1Data []byte
 }

 // Encode writes the Image m to w in JPEG 4:2:0 baseline format with the given
@@ -597,6 +598,12 @@ func Encode(w io.Writer, m image.Image, o *Options) error {
        e.buf[0] = 0xff
        e.buf[1] = 0xd8
        e.write(e.buf[:2])
+
+       // Write APP1 data if specified
+       if o != nil && o.App1Data != nil {
+               e.writeMarkerHeader(app1Marker, 2 + len(o.App1Data))
+               e.write(o.App1Data)
+       }

@rsc
Copy link
Contributor

@rsc rsc commented Oct 23, 2015

Honestly I think EXIF parsing and handlign might be best put elsewhere.

/cc @nigeltao

@rsc rsc changed the title image/jpeg: Specify APP1 segment for outputting EXIF data in jpeg.Encode()? image/jpeg: specify APP1 segment for outputting EXIF data in jpeg.Encode()? Oct 23, 2015
@rsc rsc added this to the Go1.6 milestone Oct 23, 2015
@rsc rsc added the Thinking label Oct 23, 2015
@jdeng
Copy link
Author

@jdeng jdeng commented Oct 23, 2015

Agreed.
But this is more like a bridge (or a hook). No EXIF specific logic is involved here. APP1 is like APP0 except it is widely used to store EXIF data.

@nigeltao nigeltao modified the milestones: Unplanned, Go1.6 Oct 25, 2015
@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Oct 25, 2015

I don't want to commit to a []byte-typed Options.App1Data field if it rules out having a more structured Go representation of EXIF inside the image/jpeg package itself (see also #4341). I think some bigger thinking is required around EXIF in general, for reading, modifying, and writing, before adding an incremental patch like the OP suggestion to fix one very specific use case.

For your immediate problem, it seems straightforward to me to encode to a bytes.Buffer instead of an os.File directly, and insert the APP1 data after the first 2 bytes of the buffered []byte.

Or, you can fork the image/jpeg package to encode exactly what you want.

Neither of these seem particularly difficult to me, and they don't constrain (under the Go 1.x backwards compat promise) any more general approach to the standard image/jpeg library.

@jdeng
Copy link
Author

@jdeng jdeng commented Oct 25, 2015

Wrapping up with bytes.Buffer loses the efficiency (e.g., memory consumption) and elegance of io.Writer. os.File is just one example, it could also be http.ResponseWriter.

Another possibly less intrusive (but still hacky) workaround would be adding a SkipSOI (start of image) flag in the Options struct. So that it allows people to manipulate/output all the APP segments (e.g., JFIF/APP0, EXIF/APP1 and other customized segments) without the need of holding all the data in memory or other sorts of post processing.

+ if o == nil || !o.SkipSOI {
    e.buf[0] = 0xff
    e.buf[1] = 0xd8
    e.write(e.buf[:2])
+ }
@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Oct 26, 2015

If you really want to avoid the memory cost of buffering the entire output, you could always implement your own io.Writer that wraps another io.Writer, and runs your own callback or otherwise inserts your data after the first two bytes (the "\xff\xd8" SOI marker). Again, no change to the standard library is needed.

The Options.SkipSOI proposal has the same fundamental problem as the original Options.App1Data proposal: any hacky workaround added to the standard library has to be kept for the lifetime of Go 1.x. I don't see the need for a hacky workaround in the standard library when you can just as easily hack a workaround outside the standard library.

@s3rj1k
Copy link

@s3rj1k s3rj1k commented Mar 23, 2018

any update on this?

@NirmalVatsyayan
Copy link

@NirmalVatsyayan NirmalVatsyayan commented Nov 26, 2018

+1

Any update here ?

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

Successfully merging a pull request may close this issue.

None yet
5 participants