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

Feature: implement insert orientation #18

Closed

Conversation

TonyBrobston
Copy link
Contributor

@TonyBrobston TonyBrobston commented Jul 30, 2020

Unfortunately updateOrientationCode doesn't seem to really solve my problem. After compressing an File there is no orientation is -1. So I need the ability to insert/update orientation.

It seems this may be the logic to do it: https://github.com/hMatoba/piexifjs/blob/2180d60b8cdf638e236e0a6703b7c378bb4f5785/piexif.js#L56

@TonyBrobston
Copy link
Contributor Author

TonyBrobston commented Jul 30, 2020

@ginpei Does this section make enough sense to you that you could help me implement this? I'm struggling to grasp what exactly what change needs made.

https://github.com/hMatoba/piexifjs/blob/2180d60b8cdf638e236e0a6703b7c378bb4f5785/piexif.js#L69-L71

I think I have at least the beginning of what we need for tests. I think we may want to eventually move to insertOrUpdateOrientationCode or upsertOrientationCode or setOrientationCode rather than updateOrientationCode and insertOrientationCode. I think the logic could check for orientation in the exif and if it's there, then setUint16 else insert.

@ginpei
Copy link
Owner

ginpei commented Jul 31, 2020

@TonyBrobston I'll check it this weekend.

As a quick look, what we need for the insert functionality are: 1. create the orientation segment 2. insert the segment into image. In the code you shared, the pack() looks the # 1, mergeSegments() would be # 2.

The term segment is an unit for image meta data. The structure of the JPEG file can be separated like HTML's <head> and <body>. Segments appears in the <head>-ish area and number of those are up to the file.

@ginpei
Copy link
Owner

ginpei commented Aug 2, 2020

@TonyBrobston Do you mind if I implement it? Sorry, forget this

@ginpei
Copy link
Owner

ginpei commented Aug 3, 2020

The code would be like this. To insert things, we need a new buffer since buffer cannot be extended.

function setOrientationCode () {
  // JEPG does not have exif segment
  if (!hasExifSegment()) {
    createSegment();
    appendSegment();
    return output;
  }

  // The exif segment does not have orientation entry
  if (!hasOrientationEntryInExifSegment()) {
    createEntry();
    appendEntry();
    replaceSegment();
    return output;
  }

  // OK orientation information exists
  cloneInput(); // to make output same as the other patterns
  overwriteOrientationCode();
  return output;
}

The code we have to add looks not tiny. Is it better to find replacement?

@TonyBrobston
Copy link
Contributor Author

When I have some time I'll try to implement something based on the information you provided. I'm still not confident I'll be successful, but I will try.

@TonyBrobston
Copy link
Contributor Author

@ginpei I was thinking we could grab all the code from here: https://github.com/hMatoba/piexifjs/blob/2180d60b8cdf638e236e0a6703b7c378bb4f5785/piexif.js#L56
get the test passing, then determine what all we don't need and slim it down to only what we need.

@ginpei
Copy link
Owner

ginpei commented Sep 3, 2020

@TonyBrobston Hi I'm so sorry for this long silence.

And it's hard to additionally say, but I would not like to add this feature because I'm afraid that adding it requires almost full logic to access Exif data, and results in lots of new code, which increases file size and code complexity, while alternatives exist already. (Although I haven't tried the others actually.)

If you could implement them once, and the change was small enough, I would change my mind.

@TonyBrobston
Copy link
Contributor Author

@ginpei It's alright. I'm not sure I'm capable of implementing this feature. Maybe someday in the future I will do. For now I'll close this.

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.

None yet

2 participants