-
Notifications
You must be signed in to change notification settings - Fork 201
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
[WIP] PPD option preset auto-generation for common job IPP attributes #236
base: master
Are you sure you want to change the base?
Conversation
To make the job IPP attributes "print-color-mode", "print-quality", and "print-content-optimize" actually doing "the right thing" for most printers we auto-generate PPD option presets now. CUPS already had the functionality to apply a preset of one or more PPD option settings for each of the 6 combinations of "print-color-mode" and "print-quality" settings for longer time but this was never actually being made use of as it required manually defining the presets via "APPrinterPreset" PPD attributes. This no one wants to do with the ~10000 PPDs which come with a typical Linux distribution nowadays. Even a user with his few print queues does not want to do it, know how to do it, or know that it is even possible. This commit makes this fully automatic on every PPD where the presets are not already manually defined via "APPrinterPreset" attributes. When creating the PPD cache for a queue's PPD file and the PPD is without manual presets, it calls the new function _ppdCacheAssignPresets() which analyses for each PPD option the names of the choices to find out which choices switch to manochrome or to color, lower or raise print quality, or improve text/graphics/photo printing and which are the moset suitable of them. It also analyses the PostScript/PJL code of the choices to find out whether it affects the printing resolution. With this it selects which options get into the the presets and with which choices. In addition to presets for the "print-color-mode"/"print-quality" combos presets for "print-content-optimize" are introduced. PPD Option settings added by preset are logged on job execution.
This pull request introduces 4 alerts when merging 3b07df3 into e6be109 - view on LGTM.com new alerts:
|
This especially revealed that on selecting the final setting for the color mode/quality presets only 2 of the intended 3 passes were done.
Automatic Windows test build errored on that.
The code does not build on Windows as Windows does not provide |
@tillkamppeter We can add a strcasestr emulation function - we'll need it on a number of platforms other than Windows as well... |
I have thought the same thing, already have added one, not yet committed, will do in the next 20 min or so. |
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 we need to refactor this. _ppdCreateFromIPP*
should create APPrinterPreset and cupsPrintOptimize options, and then _ppdCacheCreateWith*
should load/restore those.
"print-content-optimize" is NOT a preset mechanism, it is simply a hint to the printer to tell it what kind of content to optimize for when printing. If there are "standard" PPD options to correspond to it, we can map them (like we do for duplex and color model).
"job-presets-supported" (what APPrinterPreset corresponds to) should provide at least basic photo, normal, and draft print presets.
{ | ||
_PWG_PRINT_CONTENT_OPTIMIZE_AUTO = 0, /* print-content-optimize=auto */ | ||
_PWG_PRINT_CONTENT_OPTIMIZE_PHOTO, /* print-content-optimize=photo */ | ||
_PWG_PRINT_CONTENT_OPTIMIZE_GRAPHICS, /* print-content-optimize=graphics */ |
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.
"graphic" (singular). Some printers incorrectly report "graphics".
That said, I think we should only set this in presets for "photo". Anything else isn't even vaguely useful.
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.
OK, I have made the part of CUPS which executes jobs and there applies the presets understand both, as I was in doubt. So using singular everywhere and CUPS only understanding singular is good enough as this is the standard? The plural can simply have come from my understanding of English, assuming that one says "This document is graphics" and not "This document is graphic".
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.
@tillkamppeter The PWG standard uses singular keywords while the normal English usage allows for either. And unfortunately there are printers out there that use the plural form due to a typo in an older AirPrint specification that was later fixed. So you do need to support both... Ideally I would just map "print-content-optimize" to "cupsPrintOptimize" and then support "photo" for the photo presets and ignore the rest for APPrinterPreset.
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.
So we leave it the way that we accept both singular and plural of "graphics"?
But "cupsPrintOptimize" is only in the generated PPDs for driverless IPP printers? My intention is also to support existing PPDs for non-driverless printers as well as possible.
What do you mean with 'and then support "photo" for the photo presets and ignore the rest for APPrinterPreset.'? Where are "photo" presets? I only see the grid of 6 presets for the combos of mono/color and draft/normal/high quality.
cups/ppd-private.h
Outdated
@@ -131,6 +141,11 @@ struct _ppd_cache_s /**** PPD cache and PWG conversion data ****/ | |||
/* Number of print-color-mode/print-quality options */ | |||
cups_option_t *presets[_PWG_PRINT_COLOR_MODE_MAX][_PWG_PRINT_QUALITY_MAX]; | |||
/* print-color-mode/print-quality options */ | |||
int num_optimize_presets[_PWG_PRINT_CONTENT_OPTIMIZE_MAX]; |
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.
They aren't presets, they are print-content-optimize values.
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.
Yes, I have "invented" presets for these, to implement the IPP-attribute->Set-of-PPD-option-settings assignment. One could name them differently. My intuition was here that a "preset" is simply nothing more than as set of PPD option settings, key/value pairs, independent of by which IPP attribute(s) the preset gets selected.
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.
If you are mapping print-content-optimize, then please call it a mapping (like I do for everything else). Presets have a very specific meaning in IPP and mixing terminology just makes things more confusing.
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.
OK, will do.
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.
Renamed everything appropriately now.
@@ -214,6 +229,8 @@ extern const char *_pwgMediaTypeForType(const char *media_type, | |||
extern const char *_pwgPageSizeForMedia(pwg_media_t *media, | |||
char *name, size_t namesize) _CUPS_PRIVATE; | |||
|
|||
extern void _ppdCacheAssignPresets(ppd_file_t *ppd, _ppd_cache_t *pc) _CUPS_PRIVATE; |
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 don't think we want this here. cupsMarkOptions normally handles the PPD mapping, so let's do it there?
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.
cupsMarkOption(s) could do such an assignment, but it is much faster and more efficient if the presets are pre-generated in the PPD cache, as PPDs are static, they do not change from job to job. With the presets in the PPD cache, a cupsMarkOptions()
on print-color-mode=gray print-quality=5
would go very quickly.
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.
"gray" is not a valid IPP keyword for "print-color-mode"... The issue here is NOT speed (marking is not a significant cost, generating the PPD cache is), it is simplicity and reliability so that we can accurately map from IPP attribute names/values to PPD options/choices and back again.
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.
Sorry, it must be monochrome
. Am I right?
Do you mean that if I create a cups_option_t
list of print-color-mode=monochrome print-quality=5
that cupsMarkOptions()
called with this option list should mark the option settings according to the the preset for monochrome and high quality?
i > 0; | ||
i --, preset ++) | ||
{ | ||
if (!ippFindAttribute(job->attrs, preset->name, IPP_TAG_ZERO) && |
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 are you trying to do here? Better to:
a. Define a cupsPrintOptimize option in the PPD
b. Include "*cupsPrintOptimize Photo" for the photo preset
Then we don't have any special casing.
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.
As I told in the answer of your comment above, I do not want to modify PPD files but have a solution which works at run-time and fully automatically for the user, on existing PPDs of non-driverless printers.
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 am not asking you to modify PPD files.
If you are mapping vendor PPD options to the "print-content-optimize" attribute, then you can reflect this in the PPD cache like we do for "sides". The auto-generated IPP Everywhere PPD can use the name "cupsPrintOptimize" for the PPD option which would be one of the supported PPD option keywords that get mapped to "print-content-optimize".
What I'm thinking is that you'd add the following to the _ppd_cache_s
structure in ppd-private.h:
char *optimize_option, /* PPD option for print-content-optimize */
*optimize_values[_PWG_OPTIMIZE_MAX]; /* PPD choices for print-content-optimize values */
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.
My approach allows mapping print-content-optimize
to more than one single option in the PPD file, so it is more universally applicable.
We should give this array of five option/value lists a better name than optimizePresets
.
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.
Till, unless there are specific PPDs you plan to support, we don't need the complexity. And honestly we barely need print-content-optimize - the only useful value is 'photo' which can normally be inferred from the content. The the class of printer you are supporting via PPD files do not offer photo quality... :/
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 simply saw this IPP attribute put prominently into the web interface of PAPPL ("Prtinting Defaults") and so I thought that there are many (non-driverless, these are the ones PAPPL is for) printers somehow supporting it. And I also saw in PPD files of non-driverless printers options for content optimization, with not necessarily having the same names at every driver/manufacturer, so I tried to map these to the print-content-optimize
attribute to make it useful, especially as I created this code originally for retro-fitting PPD-based drivers (and PostScript PPDs) into PAPPL-based Printer Applications. Then later it came up the idea in me to use that code in CUPS.
Printers with PPD files are not only PostScript laser printers, there are also inkjets supported with HPLIP, Gutenprint, ... These do photo quality and they are controlled by PPD files. So users currently set them up with a PPD-file-based driver in CUPS.
I know that Gutenprint and HPLIP are actively maintained and so their maintainers should create native Printer Applications out of them, but this can take time, so I have covered them in my retro-fitting Printer Applications to work the best possible for everyone. I think to be able to convince the HPLIP guys to switch over scanning support in PAPPL needs to get finished.
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.
And by the way, in the PAPPL retro-fit library I try to auto-detect print job content, especially image file input (JPG or PNG) is considered photo and if no specific print-content optimization is selected I select "photo" then.
cups/ppd-cache.c
Outdated
@@ -891,6 +893,28 @@ _ppdCacheCreateWithFile( | |||
cupsParseOptions(valueptr, 0, | |||
pc->presets[print_color_mode] + print_quality); | |||
} | |||
else if (!_cups_strcasecmp(line, "OptimizePreset")) |
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.
Not a preset, don't use the "preset" name for it!
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 could name it differently.
cups/ppd-cache.c
Outdated
@@ -1427,6 +1452,10 @@ _ppdCacheCreateWithPPD(ppd_file_t *ppd) /* I - PPD file */ | |||
|
|||
if ((ppd_attr = ppdFindAttr(ppd, "APPrinterPreset", NULL)) != NULL) | |||
{ | |||
/* | |||
* "Classic" Mac OS approach |
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.
Use the right name ("macOS") if you are going to comment on this. But keep in mind that we support APPrinterPreset on Linux/BSD/etc. as well (exposed as job-presets-supported for IPP Everywhere).
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.
Sorry, I could reword it into simply "Classic" approach
.
{ | ||
if (_cups_strcasecmp(c, "1rhit") == 0) | ||
properties->sets_high = 7; | ||
else if (_cups_strcasecmp(c, "6rhit") == 0) |
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.
Don't try to do so much with presets. Keep them simple, otherwise there are just too many presets!
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 we need to refactor this. _ppdCreateFromIPP* should create APPrinterPreset and cupsPrintOptimize options, and then _ppdCacheCreateWith* should load/restore those.
My change is not meant for the PPDs generated for driverless printers, but for the > 10000 PPDs already out there for PostScript printers and classic CUPS drivers. No one ever will hand-add the APPrinterPreset and cupsPrintOptimize PPD attributes to those, so I looked for a method to do this fully automatically, on-the-fly, on run-time, without need to modify the original PPD files, not even the queue's ones in /etc/cups/ppd
.
"print-content-optimize" is NOT a preset mechanism, it is simply a hint to the printer to tell it what kind of content to optimize for when printing. If there are "standard" PPD options to correspond to it, we can map them (like we do for duplex and color model).
I liked the mechanism that there is a preset (as of a set of PPD option settings) assigned to each combo of print-color-mode and print-quality, as that makes printing standardised for all printers and removes the need of exposing the PPD options to the user. I disliked that PPDs do not come with presets, so I started this algorithm for auto-discovering which are the best option settings for each of the combos of the 2 IPP attributes. I did it in libppd in cups-filters, as I wanted to use it in the retro-fitting Printer Applications.
Then I saw that there is also the print-content-optimize attribute in the web interface of PAPPL (Printer Defaults) and thought that I create a similar algorithm for it. As the ppd-cache.c which I had overtaken from libcups into libppd does not select presets based on this one, I introduced an additional set of presets for this IPP attribute. Here the algorithm finds content-optimizing options in the PPD and adds them to these presets.
After this all working well in the Printer Applications and remembering that ppd-cache.c has its origin in CUPS I decided to port my work to CUPS, and have it already as distro patch in the CUPS of Impish.
"job-presets-supported" (what APPrinterPreset corresponds to) should provide at least basic photo, normal, and draft print presets.
I am working with existing PPD files of non-driverless printers here, so I cannot poll the presets from the printer. I have to analyse the PPDs to find what is best for presets.
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.
The lots of rules which some very specific ones come from my testing on existing PPD files. The one you cite is for Ricoh, and they put out very many PPDs for their professional printers, several PPDs (for the different PDLs) for each new model and also fixes for existing PPDs).
I tested several for PostScript (HP, Ricoh, Toshiba, ...), HPLIP, Gutenprint, Foomatic, ... and this made me adding some "quirk" rules to the originally written up ones.
Do not think that in the future we have to update these rules all the time for the new PPDs. As PPDs are deprecated manufacturers an driver developers will hopefully not continue developing on them and squeeze in their required functionality with more and more adventurous workarounds. The rules are a good set for handling the legacy of PPDs and giving the user a modern IPP printing experience.
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.
Till, presets are most useful for common combinations of options. Complex printers could easily provide thousands of potential "presets" with your current code, and not all of those combinations are valid (need to check for constraints, which on these devices are also complex and heavily optimized by me years ago).
I agree that we should do everything we can to offer consistency to users for all printers, but that functionality/consistency is targeted at general users and not the "experts" that are using those high-end printers.
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.
For the experts I always leave the possibility to set the options manually. If an option is supplied with the job, it will not get auto-set by the pre-set.
Do you have some sample PPDs where there are complex constraints which could interfere with auto-created pre-sets? I have only seen constraints with installable accessories, or constraints with paper source/paper type, paper size/duplex, like duplex on transparencies or A3 paper from envelope tray, not on resolution,. toner saving, resolution enhancement, ...
Windows does not have a strcasestr() so we have to provide our own implementation, as we already did with strcasecmp() and strncasecmp().
I have converted the And now the code builds under Windows! |
This is all for all these existing PPD files dying a nicer death in the last 1 or 2 years on PPD-supporting CUPS 2.4.x ... |
… to check for Checking the PPD file for the HP LaserJet 4050 (PostScript) found more option/choice names of options which influence print quality: - Smoothing - Halftone - ProRes(600/1200/2400) Some other of HPLIP's PostScript PPDs use "HPPJLColorAsGray" with choices "yes" and "no" instead of ""HPColorAsGray" with "True" and "False". libcupsfilters: More PPD preset rules, especially for HP Also analyse choice names for options which contain "Resolution" in their names.
This way we also cover "cupsColorMode" for example which appears in auto-generated PPDs for driverless IPP printers (in case we cannot print directly to the printer but only have access via a CUPS queue). Also updated a comment and a log message.
@tillkamppeter it looks there are some discrepancies in the PR which Mike mentioned and haven't been tackled yet, so I'll move it to 2.5 as it is a big bunch of new code. |
OK, @zdohnal let us aim for 2.5.0 with it ... |
…ies record Static analysis of code, GitHub code scanning, found missing NULL checks for allocating memory for option choice properties records (gives NULL when out of memory) and for grabbing the records from a CUPS array (should now never happen as we now skip the option on the first memory allocation failure). Added appropriate NULL checks. Now in case of unsufficient memory the current option gets skipped on the first failed calloc() call.
Renamed the fields in _ppd_cache_s and updated the comments.
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.
Still really not happy with these changes. The added complexity and number of presets you will introduce for high-end printers will only cause problems for users and is not something that will be preserved in CUPS 3.0 anyways...
Some general feedback:
- You need to update the cache version number in ppd-private.h, otherwise existing cache files will not be regenerated.
- The print-content-optimize enumerations still don't follow the registered keyword names.
Ultimately, what problem are you trying to solve with these changes?
I know, did it in the beginning but now I need to do it again ...
OK, I will correct that.
The problem is the following: If we have a CUPS queue with a PPD file, desktop print dialogs using libcups show all the options which the PPD provides, so the user can adjust them to the job's needs. Now, if we share the printer CUPS makes it available via IPP, as a driverless IPP printer, like a Printer Application. Now there can be IPP clients which are not CUPS clients (mobile phones, Windows, ...). They show only standard IPP options in their print dialogs, especially the options So my idea here was that if there are no manually created presets in the PPD file ( The algorithm is based on existing PPD files, and in the age of CUPS drivers being replaced by Printer Applications, and PostScript printers being replaced by driverless IPP printers not many new PPD files will appear. So there will be not many new PPDs where the algorithm is missing options due to new names. I created this already some time ago and deployed it in the PPD-retro-fitting Printer Applications and with this PR I thought to extend it to CUPS for the few years (now only months?) left until the switchover to CUPS 3.x (in Ubuntu I had applied it already as distro patch). |
To make the job IPP attributes "print-color-mode", "print-quality", and "print-content-optimize" actually doing "the right thing" for most printers we auto-generate PPD option presets now.
CUPS already had the functionality to apply a preset of one or more PPD option settings for each of the 6 combinations of "print-color-mode" and "print-quality" settings for longer time but this was never actually being made use of as it required manually defining the presets via "APPrinterPreset" PPD attributes. This no one wants to do with the ~10000 PPDs which come with a typical Linux distribution nowadays. Even a user with his few print queues does not want to do it, know how to do it, or know that it is even possible.
This way printing gets easier for users, as once they can print from simple print dialogs only offering stanard IPP options (and not the PPD option) like on phones or IoT devices, and second. they can print with the same standard options on any printer. With the
lp
command one can simply use a few options which work everywhere:This commit makes this fully automatic on every PPD where the presets are not already manually defined via "APPrinterPreset" attributes.
When creating the PPD cache for a queue's PPD file and the PPD is without manual presets, it calls the new function _ppdCacheAssignPresets() which analyses for each PPD option the names of the choices to find out which choices switch to manochrome or to color, lower or raise print quality, or improve text/graphics/photo printing and which are the moset suitable of them. It also analyses the PostScript/PJL code of the choices to find out whether it affects the printing resolution.
With this it selects which options get into the the presets and with which choices.
In addition to presets for the "print-color-mode"/"print-quality" combos presets for "print-content-optimize" are introduced.
PPD Option settings added by preset are logged on job execution.