Skip to content

PDFBOX-6032: add configurable default image factory #210

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

Closed

Conversation

kimilg
Copy link

@kimilg kimilg commented Jul 12, 2025

  • This PR addresses PDFBOX-6032 by introducing a new overload of PDImageXObject#createFromByteArray that accepts a user-supplied defaultFactory parameter for customizable image conversion behavior.
  • The goal is to allow users to provide a defaultFactory for custom image conversion. This factory is used directly for formats like BMP and GIF, and acts as a fallback for PNG and TIFF when the optimized conversion path fails and would otherwise fall back to LosslessFactory.

PDFBOX-6032: change test method name
* PDImageXObject.
* @throws IllegalArgumentException if the image type is not supported.
*/
public static PDImageXObject createFromByteArray(PDDocument document, byte[] byteArray, String name, DefaultFactory defaultFactory) throws IOException
Copy link

Choose a reason for hiding this comment

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

Here I'm a bit confused, also by the naming stuff.

I would rather extend the existing method with a parameter. I think "DefaultFactory" is a bad name. I.e., the way I would implement this would be

// Method with signature of the existing method is just forwarding to the other  new overload
public static PDImageXObject createFromByteArray(PDDocument document, byte[] byteArray, String name)
{
return createFromByteArray(document,byteArray,name, null);
}

// Extend the existing method with the one parameter
public static PDImageXObject createFromByteArray(PDDocument document, byte[] byteArray, String name, DefaultFactory defaultFactory) throws {
  // .. logic like it is now BUT instead of just encoding this lossless we first check if we have a *fallback*

if( defaultFactory != null ) 
  return defaultFactory.create...(...)
// otherwise just do the PNGEncoder Lossless stuff like it is now.
}

And therefor I think defaultFactory is bad name. It should be a fallback.

Copy link
Contributor

@lehmi lehmi Jul 13, 2025

Choose a reason for hiding this comment

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

I've proposed DefaultFactory and I'm not really happy with that name.

Fallback won't be correct as it isn't a fallback in all cases. It is for png and tiff which can't be processed but it isn't for bmp and gif.

How about CustomFactory?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback.
I’ve removed the duplicate logic, and the code feels more compact and cleaner now.
Naming the functional interface is tricky - a precise name gets too long, but a short one feels too vague.
I renamed it to CustomFactory because it more clearly reflects its intended usage than DefaultFactory.

PDFBOX-6032: change test comment

PDFBOX-6032: change format in createFromByteArray
@kimilg kimilg force-pushed the PDFBOX-6032-configurable-image-factory branch from 1f2c526 to 28ca17d Compare July 14, 2025 02:58
import org.apache.pdfbox.pdmodel.PDDocument;

@FunctionalInterface
public interface CustomFactory {
Copy link

Choose a reason for hiding this comment

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

Some JavaDoc to describe what this interface is about would be nice.

But other than that, the changes look fine for me.

Copy link
Author

Choose a reason for hiding this comment

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

Right, I forgot to add it earlier. I've now added a JavaDoc comment to this class.

@THausherr
Copy link
Contributor

Thanks for the PR; I forgot the magic words in the commit message, please close the PR yourself.

@kimilg kimilg closed this Jul 16, 2025
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.

4 participants