You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "andrewmkhoury (via GitHub)" <gi...@apache.org> on 2023/07/08 22:57:42 UTC
[GitHub] [commons-imaging] andrewmkhoury opened a new pull request, #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
andrewmkhoury opened a new pull request, #301:
URL: https://github.com/apache/commons-imaging/pull/301
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] andrewmkhoury commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1258469532
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
π `stopReadingBeforeImageData` or works. I will see about adding a unit test as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [IMAGING-355] Large animated GIF takes too much heap memory in getMetadata [commons-imaging]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #301:
URL: https://github.com/apache/commons-imaging/pull/301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] andrewmkhoury commented on pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#issuecomment-1672311589
@garydgregory and @kinow I implemented the changes per your request. However, I am not sure how much value passing stopReadinngBeforeImageData adds. getImageInfo already has a unit test and I added a test for getMetadata.
* Add stopReadingBeforeImageData to GifImageParameters
* Add unit test for IMAGING-355
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] codecov-commenter commented on pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#issuecomment-1627539853
## [Codecov](https://app.codecov.io/gh/apache/commons-imaging/pull/301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#301](https://app.codecov.io/gh/apache/commons-imaging/pull/301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a4983f3) into [master](https://app.codecov.io/gh/apache/commons-imaging/commit/038666a450fbcb7388407807191047855d3601b2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (038666a) will **not change** coverage.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #301 +/- ##
=========================================
Coverage 70.81% 70.81%
Complexity 3405 3405
=========================================
Files 333 333
Lines 16828 16828
Branches 2588 2588
=========================================
Hits 11916 11916
Misses 3897 3897
Partials 1015 1015
```
| [Impacted Files](https://app.codecov.io/gh/apache/commons-imaging/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [...he/commons/imaging/formats/gif/GifImageParser.java](https://app.codecov.io/gh/apache/commons-imaging/pull/301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW1hZ2luZy9mb3JtYXRzL2dpZi9HaWZJbWFnZVBhcnNlci5qYXZh) | `77.07% <100.00%> (ΓΈ)` | |
:mega: Weβre building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1257503334
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
Maybe this `true`/`false` (`stopBeforeImageData`) should instead be retrieved from the `GitImagingParameters` object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] garydgregory commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1257659022
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
**Note**: All current callers of `org.apache.commons.imaging.formats.gif.GifImageParser.readFile(ByteSource, boolean)` pass false for `stopBeforeImageData`.
Would it make for all call sites to pick up this value from `GifImagingParameters`? Or, would this cause some use-cases to break. We will need to make sure to unit test whatever code we change or add.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata [commons-imaging]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1435633510
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,11 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ boolean stopReadingBeforeImageData = true;
Review Comment:
You've changed the default behavior here which makes the parameter a forced usage, which might be a surprise, I'll flip it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] garydgregory commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1257506692
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
That's a good point!
I wonder if this data is hard-coded to be skipped it will be unavailable for other API calls once the object is constructed. That would be bad. But if it can be paramerized, then the client call.site can decide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata [commons-imaging]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1435633510
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,11 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ boolean stopReadingBeforeImageData = true;
Review Comment:
You've changed the default behavior here which makes the parameter a forced usage, which might be a surprised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] andrewmkhoury commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1258469532
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
π `stopReadingBeforeImageData` works. I will see about adding a unit test as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] andrewmkhoury commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1258469532
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
`stopReadingBeforeImageData` or `skipImageData` works.
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
π `stopReadingBeforeImageData` or `skipImageData` works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [IMAGING-355] Large animated GIF takes too much heap memory in getMetadata [commons-imaging]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1435633510
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,11 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ boolean stopReadingBeforeImageData = true;
Review Comment:
You've changed the default behavior here which makes the parameter a forced usage, which might be a surprise, I'll flip it post mege.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] garydgregory commented on a diff in pull request #301: IMAGING-355 - Large animated GIF takes too much heap memory in getMetadata
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #301:
URL: https://github.com/apache/commons-imaging/pull/301#discussion_r1257659022
##########
src/main/java/org/apache/commons/imaging/formats/gif/GifImageParser.java:
##########
@@ -407,7 +407,7 @@ public byte[] getIccProfileBytes(final ByteSource byteSource, final GifImagingPa
@Override
public ImageInfo getImageInfo(final ByteSource byteSource, final GifImagingParameters params)
throws ImagingException, IOException {
- final GifImageContents blocks = readFile(byteSource, false);
+ final GifImageContents blocks = readFile(byteSource, true);
Review Comment:
**Note**: All current callers of `org.apache.commons.imaging.formats.gif.GifImageParser.readFile(ByteSource, boolean)` pass false for `stopBeforeImageData`.
Would it make for all call sites to pick up this value from `GifImagingParameters`[1]? Or, would this cause some use-cases to break. We will need to make sure to unit test whatever code we change or add.
[1] We would add a `stopBeforeImageData` to `GifImagingParameters`, with maybe a better name like `stopReadingBeforeImageData`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org