-
Notifications
You must be signed in to change notification settings - Fork 248
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
Support for JBIG2 (port of https://github.com/apache/pdfbox-jbig2) #338
base: master
Are you sure you want to change the base?
Conversation
Thanks for this contribution. I'm probably not going to get time to do a review until next weekend, sorry for the delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the hard work here and sorry it has taken so long to review, it's a bit large for one person to feasibly review but other than scanning for obvious security flaws I'm not too worried about any style quibbles since it's all pseudo-Java-ish but isolated in the JBIG folders.
The only comments I'd prefer to address prior to merging are the 2 test files with the scary licensing terms and the changes in XObjectFactory.
@@ -0,0 +1,21 @@ | |||
============================================================================ | |||
The files sampledata_pageN.jb2 and sampledata.jb are included subject to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we by any chance remove the test files in question please? I'm never sure with copyright and licensing but the non-commercial clause in this license makes me anxious.
/// <summary> | ||
/// Interface for all JBIG2 dictionaries segments. | ||
/// </summary> | ||
internal interface IDictionary : ISegmentData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe make this a more JBIG specific name, eg. IJbigDictionary
since it could easily be confused with relating to the DictionaryToken
type.
/// <returns>A list of <see cref="Bitmap"/>s as a result of the decoding process of dictionary segments.</returns> | ||
/// <exception cref="InvalidHeaderValueException">if the segment header value is invalid.</exception> | ||
/// <exception cref="IntegerMaxValueException">if the maximum value limit of an integer is exceeded.</exception> | ||
/// <exception cref="System.IO.IOException">if an underlying IO operation fails.</exception> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is specific to Java code and we wouldn't expect an IOException
to be produced unless this is doing some file system access?
} | ||
} | ||
|
||
internal static string bitPattern(int v, int len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java naming 🙈
|
||
var bounds = xObject.AppliedTransformation.Transform(new PdfRectangle(new PdfPoint(0, 0), new PdfPoint(1, 1))); | ||
|
||
var width = dictionary.Get<NumericToken>(NameToken.Width, pdfScanner).Int; | ||
var height = dictionary.Get<NumericToken>(NameToken.Height, pdfScanner).Int; | ||
var width = dictionary.Get<NumericToken>(NameToken.Width).Int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for removing the overload using pdfScanner
in this file? In general overloads of Get
and TryGet
parsing pdfScanner
or DirectObjectFinder.Get
are required because most tokens in PDF documents can be indirect references to other tokens, in this case an Indirect reference to a number token in its own object.
Usually where they are used it's because there was already a bug in the code which meant they needed to be added, I just want to check we're not causing a regression in this file 🙏
I can implement the necessary changes if need be, also saw that it was using |
@EliotJones @kasperdaff as it would be sad to lose this work, I will create a new PR by cherry picking the work and adding the changes |
No description provided.