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

NIFI-5355 ResizeImage Fails to read PNG type on some OS's #2854

Closed
wants to merge 1 commit into from

Conversation

patricker
Copy link
Contributor

Updated ResizeImage to better support PNG file format on all platforms.
Updated resize logic to provide a higher performance/faster experience. Unfortunately Hint values are not exactly the same. I mapped them as best as possible to avoid any breaking changes.

Added in Unit tests for these changes, and also for if the requested resize size is larger than the original image.

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
  • Have you written or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
  • If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

@patricker
Copy link
Contributor Author

@JPercivall (in case you want to take a look)

@@ -17,8 +17,7 @@

package org.apache.nifi.processors.image;

import java.awt.Graphics2D;
import java.awt.Image;
import java.awt.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to explicitly reference the items we wish to import

@patricker
Copy link
Contributor Author

Thanks Joe, I got a new computer and some of my rules aren't setup correctly in the IDE yet.

@@ -196,4 +187,53 @@ public void process(final InputStream rawIn, final OutputStream out) throws IOEx
}
}

// https://stackoverflow.com/questions/7951290/re-sizing-an-image-without-losing-quality
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is polite to cite the source. however, stackoverflow posts cannot be copied as their origin/licensing is not established or clear. The code to perform the function you desire needs to be original/authentic or sourced to ALv2 or category-A compliant code https://www.apache.org/legal/resolved.html#category-a

We cannot accept this code as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joewitt StackOverflow has the following disclaimer:

site design / logo © 2018 Stack Exchange Inc; user contributions licensed under cc by-sa 3.0 with attribution required. rev 2018.7.5.30950

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah, it's possible that the code was stolen from somewhere else, but the prima facie licensing of that content is CC share alike. From what I read on the licensing page, it's only CC non commercial that is problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my research notes below. Also updated with correct original source URL, and fixed checkstyle issues.

@patricker
Copy link
Contributor Author

Some further research.

Original source code location: https://web.archive.org/web/20080516181120/http://today.java.net/pub/a/today/2007/04/03/perils-of-image-getscaledinstance.html

The code has no listed license. So according to that website:

https://web.archive.org/web/20080512032235/http://java.net/terms.csp

Source Code Submissions. You agree that any source code You contribute to a Project will be submitted under, and subject to, the license posted for that Project. If no license is posted, You agree that Your Submission will be governed by the Apache License, Version 2.0, which terms can be found at http://opensource.org/licenses/apache2.0.php. You acknowledge that You are responsible for including all applicable copyright notices and licenses with Your Submissions, and that You assume the risks of failing to do so, including the potential loss of Your rights to Your Submissions.

@joewitt
Copy link
Contributor

joewitt commented Jul 9, 2018

Thanks @patricker . I understand your point about its likely licensing. However, I'm not sure that it is sufficient for Apache Software Foundation project purposes. I've read threads or comments in the past that call these into question. Some research will be needed with ASF guidelines and we might need to ask legal if this isn't already asked.

@joewitt
Copy link
Contributor

joewitt commented Jul 9, 2018

@patricker I'm not finding good guidance in apache legal JIRA or the apache list archives yet here.

However, lets be clear what we're evaluating.

Initially this is code from Stackoverflow which we believe based on SO guidance is 'CC SA 3.0 w/Attribution' which is allowed in Apache projects according to https://www.apache.org/legal/resolved.html#cc-sa though that does't clarify if it is consider Cat A or Cat B ( a being permitted for source OR binary where B is permitted only for binary). So we'd have to clarify.

But, upon further review you found in the wayback/archives the original source is considered to be AL v2.0 code based on a default legal disclaimer for that now defunct website also in wayback/archives.

I'm not saying I disagree with your conclusion. But, one of our most critical jobs in the community is to produce legal/valid releases of source code. This is at the very least murky. We should file a LEGAL discuss thread or JIRA for resolution on this one so we can be sure we're doing the right thing. It isn't clear to me how we'd handle it and track this going forward or which license we'd claim it under/etc..

Best case is to write this code originally...
Next best is to generate a legal thread to get legal ASF approval/decision on this with guidance how to handle it.

@patricker
Copy link
Contributor Author

@joewitt I've simplified my code change to just address the actual bug/problem, no longer uses any of the code that was under question.

@joewitt
Copy link
Contributor

joewitt commented Jul 9, 2018

ok cool thanks. assuming travis is happy i'd be a +1

@pvillard31
Copy link
Contributor

I'm a +1 as well, merging to master, thanks @patricker @joewitt

@asfgit asfgit closed this in 1963697 Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants