[sane-devel] [PATCH] Add support to snapscan for Benq Scanwit 2720S film scanner

Andrew Goodbody ajg02 at elfringham.co.uk
Mon Feb 25 13:54:33 UTC 2013


On 25/02/13 11:07, Paul Menzel wrote:
> Dear Andrew,
>
>
> thank you for working on finishing the support.
>
> Am Montag, den 25.02.2013, 10:00 +0000 schrieb Andrew Goodbody:
>
> […]
>
> What is the current status?

I would call it at least 'Good'. I have used this code to scan over a 
hundred negatives. But then only I have used it. Others may want 
something else. I am open to adding to it although I would prefer to get 
an initial implementation committed before doing so.

> My comments are inline.

And my answers also.

>> ---
>>   backend/snapscan-options.c |  223 +++++++++++++++++++++++++++++---
>>   backend/snapscan-scsi.c    |  309 ++++++++++++++++++++++++++++++++++----------
>>   backend/snapscan-sources.c |   31 ++++-
>>   backend/snapscan.c         |  133 ++++++++++++++-----
>>   backend/snapscan.h         |   28 +++-
>>   5 files changed, 597 insertions(+), 127 deletions(-)
>>
>> diff --git a/backend/snapscan-options.c b/backend/snapscan-options.c
>> index a7401bb..674c8d2 100644
>> --- a/backend/snapscan-options.c
>> +++ b/backend/snapscan-options.c
>> @@ -1,9 +1,9 @@
>>   /* sane - Scanner Access Now Easy.
>>
>> -   Copyright (C) 1997, 1998, 2001 Franck Schnefra, Michel Roelofs,
>> +   Copyright (C) 1997, 1998, 2001, 2013 Franck Schnefra, Michel Roelofs,
>>      Emmanuel Blot, Mikko Tyolajarvi, David Mosberger-Tang, Wolfgang Goeller,
>>      Petter Reinholdtsen, Gary Plewa, Sebastien Sable, Mikael Magnusson,
>> -   Oliver Schwartz and Kevin Charter
>> +   Andrew Goodbody, Oliver Schwartz and Kevin Charter
>>
>>      This file is part of the SANE package.
>>
>> @@ -67,11 +67,15 @@
>>   static SANE_Int def_rgb_lpr = 4;
>>   static SANE_Int def_gs_lpr = 12;
>>   static SANE_Int def_bpp = 8;
>> +static SANE_Int def_frame_no = 1;
>>
>>
>>   /* predefined preview mode name */
>>   static char md_auto[] = "Auto";
>>
>> +/* predefined focus mode name */
>> +static char md_manual[] = "Manual";
>> +
>>   /* predefined scan mode names */
>>   static char md_colour[] = SANE_VALUE_SCAN_MODE_COLOR;
>>   static char md_bilevelcolour[] = SANE_VALUE_SCAN_MODE_HALFTONE;
>> @@ -103,6 +107,15 @@ static char lpr_desc[] = SANE_I18N(
>>       "a scan; if it's set too high, X-based frontends may stop responding "
>>       "to X events and your system could bog down.");
>>
>> +static char frame_desc[] = SANE_I18N(
>> +    "Frame number of media holder that should be scanned.");
>> +
>> +static char focus_mode_desc[] = SANE_I18N(
>> +    "Use manual or automatice selection of focus point.");
>
> 1. automatic

Done, thanks.

>> +
>> +static char focus_desc[] = SANE_I18N(
>> +    "Focus point for scanning.");
>> +
>>   /* ranges */
>>   static const SANE_Range x_range_fb =
>>   {
>> @@ -174,6 +187,16 @@ static const SANE_Range y_range_tpo_2580 =
>>       SANE_FIX (0.0), SANE_FIX (80.0), 0
>>   };        /* mm */
>>
>> +/* TPO range for the Scanwit 2720S */
>
> TPO? Turn page over [1] ? Could you enlighten me please? It’s used for
> the other devices too and search the Web did not help.
>
> [1] http://en.wikipedia.org/wiki/TPO
>
> Found it in `snapscan.h`.
>
>          typedef enum
>          {
>              SRC_FLATBED = 0,    /* Flatbed (normal) */
>              SRC_TPO,            /* Transparency unit */
>              SRC_ADF
>          } SnapScan_Source;
>
>> +static const SANE_Range x_range_tpo_2720s =
>> +{
>> +    SANE_FIX (0.0), SANE_FIX (23.6), 0
>> +};        /* mm */
>> +static const SANE_Range y_range_tpo_2720s =
>> +{
>> +    SANE_FIX (0.0), SANE_FIX (35.7), 0
>> +};        /* mm */
>> +
>>   /* TPO range for the Epson 3490 */
>>   static const SANE_Range x_range_tpo_3490 =
>>   {
>> @@ -198,6 +221,14 @@ static const SANE_Range lpr_range =
>>   {
>>       1, 50, 1
>>   };
>> +static const SANE_Range frame_range =
>> +{
>> +    1, 6, 1
>> +};
>> +static const SANE_Range focus_range =
>> +{
>> +    0, 0x300, 6
>> +};
>>
>>   static const SANE_Range brightness_range =
>>   {
>> @@ -243,6 +274,8 @@ static void init_options (SnapScan_Scanner * ps)
>>           {10, 50, 75, 100, 150, 200, 300, 400, 600, 800, 1600};
>>       static SANE_Word resolutions_2400[] =
>>           {10, 50, 75, 100, 150, 200, 300, 400, 600, 1200, 2400};
>> +    static SANE_Word resolutions_2700[] =
>> +        {4, 337, 675, 1350, 2700};
>>       static SANE_Word resolutions_3200[] =
>>           {15, 50, 150, 200, 240, 266, 300, 350, 360, 400, 600, 720, 800, 1200, 1600, 3200};
>>       static SANE_String_Const names_all[] =
>> @@ -253,6 +286,8 @@ static void init_options (SnapScan_Scanner * ps)
>>           {md_auto, md_colour, md_bilevelcolour, md_greyscale, md_lineart, NULL};
>>       static SANE_String_Const preview_names_basic[] =
>>           {md_auto, md_colour, md_greyscale, md_lineart, NULL};
>> +    static SANE_String_Const focus_modes[] =
>> +        {md_auto, md_manual, NULL};
>>       static SANE_Int bit_depth_list[4];
>>       int bit_depths;
>>       SANE_Option_Descriptor *po = ps->options;
>> @@ -287,6 +322,10 @@ static void init_options (SnapScan_Scanner * ps)
>>              y_range_tpo = y_range_tpo_2480;
>>           }
>>           break;
>> +    case SCANWIT2720S:
>> +        x_range_tpo = x_range_tpo_2720s;
>> +        y_range_tpo = y_range_tpo_2720s;
>> +        break;
>>       case PERFECTION3490:
>>           x_range_tpo = x_range_tpo_3490;
>>           y_range_tpo = y_range_tpo_3490;
>> @@ -318,6 +357,7 @@ static void init_options (SnapScan_Scanner * ps)
>>       po[OPT_MODE_GROUP].cap = 0;
>>       po[OPT_MODE_GROUP].constraint_type = SANE_CONSTRAINT_NONE;
>>
>> +    ps->res = DEFAULT_RES;
>
> Why move this from below?

To allow a different value to be set for the 2720S.

>>       po[OPT_SCANRES].name = SANE_NAME_SCAN_RESOLUTION;
>>       po[OPT_SCANRES].title = SANE_TITLE_SCAN_RESOLUTION;
>>       po[OPT_SCANRES].desc = SANE_DESC_SCAN_RESOLUTION;
>> @@ -354,11 +394,17 @@ static void init_options (SnapScan_Scanner * ps)
>>       case PERFECTION3490:
>>           po[OPT_SCANRES].constraint.word_list = resolutions_3200;
>>           break;
>> +    case SCANWIT2720S:
>> +        po[OPT_SCANRES].constraint.word_list = resolutions_2700;
>> +        ps->val[OPT_SCANRES].w = 1350;
>> +        ps->res = 1350;
>> +        break;
>>       default:
>>           po[OPT_SCANRES].constraint.word_list = resolutions_600;
>>           break;
>>       }
>> -    ps->res = DEFAULT_RES;
>> +    DBG (DL_OPTION_TRACE,
>> +        "sane_init_options resolution is %d\n", ps->res);
>>
>>       po[OPT_PREVIEW].name = SANE_NAME_PREVIEW;
>>       po[OPT_PREVIEW].title = SANE_TITLE_PREVIEW;
>> @@ -487,8 +533,18 @@ static void init_options (SnapScan_Scanner * ps)
>>           source_list[i] = 0;
>>           po[OPT_SOURCE].size = max_string_size(source_list);
>>           po[OPT_SOURCE].constraint.string_list = source_list;
>> -        ps->source = SRC_FLATBED;
>> -        ps->source_s = (SANE_Char *) strdup(src_flatbed);
>> +        if (ps->pdev->model == SCANWIT2720S)
>> +        {
>> +            ps->source = SRC_TPO;
>> +            ps->source_s = (SANE_Char *) strdup(src_tpo);
>> +            ps->pdev->x_range.max = x_range_tpo.max;
>> +            ps->pdev->y_range.max = y_range_tpo.max;
>> +        }
>> +        else
>> +        {
>> +            ps->source = SRC_FLATBED;
>> +            ps->source_s = (SANE_Char *) strdup(src_flatbed);
>> +        }
>>       }
>>
>>       po[OPT_GEOMETRY_GROUP].title = SANE_I18N("Geometry");
>> @@ -581,12 +637,24 @@ static void init_options (SnapScan_Scanner * ps)
>>       case PERFECTION3490:
>>           bit_depth_list[++bit_depths] = 16;
>>           break;
>> +    case SCANWIT2720S:
>> +        bit_depth_list[bit_depths] = 12;
>> +        break;
>>       default:
>>           break;
>>       }
>>       bit_depth_list[0] = bit_depths;
>>       po[OPT_BIT_DEPTH].constraint.word_list = bit_depth_list;
>> -    ps->val[OPT_BIT_DEPTH].w = def_bpp;
>> +    if (ps->pdev->model == SCANWIT2720S)
>> +    {
>> +        ps->val[OPT_BIT_DEPTH].w = 12;
>> +        ps->bpp_scan = 12;
>> +    }
>> +    else
>> +    {
>> +        ps->val[OPT_BIT_DEPTH].w = def_bpp;
>> +        ps->bpp_scan = def_bpp;
>> +    }
>
> For me it would be more clear, if you set def_bpp depending on the
> device (in the beginning?) and just add `ps->bpp_scan = def_bpp;`. Why
> is that needed. Bug fix?

Ah, OK, good thought. I was just trying for minimal changes. I also was 
concerned if init_options was always called first or at all.
Yes, setting bpp_scan is a bug fix. Without doing so bpp_scan is not set 
before first scan and caused bogus values to be passed for the preview. 
This caused the 2720S to lock up.

>>
>
> Whitespace errors. Check with `git show` (and set up colors for Git).

Thanks, will do.

>>       po[OPT_QUALITY_CAL].name = SANE_NAME_QUALITY_CAL;
>>       po[OPT_QUALITY_CAL].title = SANE_TITLE_QUALITY_CAL;
>> @@ -813,6 +881,45 @@ static void init_options (SnapScan_Scanner * ps)
>>       po[OPT_THRESHOLD].constraint.range = &positive_percent_range;
>>       ps->threshold = DEFAULT_THRESHOLD;
>>
>> +    po[OPT_FRAME_NO].name = SANE_I18N("Frame");
>> +    po[OPT_FRAME_NO].title = SANE_I18N("Frame to be scanned");
>> +    po[OPT_FRAME_NO].desc = frame_desc;
>> +    po[OPT_FRAME_NO].type = SANE_TYPE_INT;
>> +    po[OPT_FRAME_NO].unit = SANE_UNIT_NONE;
>> +    po[OPT_FRAME_NO].size = sizeof (SANE_Int);
>> +    po[OPT_FRAME_NO].cap = SANE_CAP_SOFT_SELECT
>> +                       | SANE_CAP_SOFT_DETECT
>> +                       | SANE_CAP_INACTIVE;
>> +    po[OPT_FRAME_NO].constraint_type = SANE_CONSTRAINT_RANGE;
>> +    po[OPT_FRAME_NO].constraint.range = &frame_range;
>> +    ps->frame_no = def_frame_no;
>> +
>> +    po[OPT_FOCUS_MODE].name = SANE_I18N("Focusmode");
>> +    po[OPT_FOCUS_MODE].title = SANE_I18N("Auto or manual focus");
>> +    po[OPT_FOCUS_MODE].desc = focus_mode_desc;
>> +    po[OPT_FOCUS_MODE].type = SANE_TYPE_STRING;
>> +    po[OPT_FOCUS_MODE].unit = SANE_UNIT_NONE;
>> +    po[OPT_FOCUS_MODE].size = 16;
>> +    po[OPT_FOCUS_MODE].cap = SANE_CAP_SOFT_SELECT
>> +                       | SANE_CAP_SOFT_DETECT
>> +                       | SANE_CAP_INACTIVE;
>> +    po[OPT_FOCUS_MODE].constraint_type = SANE_CONSTRAINT_STRING_LIST;
>> +    po[OPT_FOCUS_MODE].constraint.string_list = focus_modes;
>> +    ps->focus_mode_s= md_auto;
>> +    ps->focus_mode = MD_AUTO;
>> +
>> +    po[OPT_FOCUS_POINT].name = SANE_I18N("Focuspoint");
>
> Focus_point? Not sure what name is used for though.

Changed to Focus-point and also Focus-mode above. This is in keeping 
with other names in the file. name is used to identify the option in the 
saved settings files, not normally meant to be read by humans.

>> +    po[OPT_FOCUS_POINT].title = SANE_I18N("Focus point");
>> +    po[OPT_FOCUS_POINT].desc = focus_desc;
>> +    po[OPT_FOCUS_POINT].type = SANE_TYPE_INT;
>> +    po[OPT_FOCUS_POINT].unit = SANE_UNIT_NONE;
>> +    po[OPT_FOCUS_POINT].size = sizeof (SANE_Int);
>> +    po[OPT_FOCUS_POINT].cap = SANE_CAP_SOFT_SELECT
>> +                       | SANE_CAP_SOFT_DETECT
>> +                       | SANE_CAP_INACTIVE;
>> +    po[OPT_FOCUS_POINT].constraint_type = SANE_CONSTRAINT_RANGE;
>> +    po[OPT_FOCUS_POINT].constraint.range = &focus_range;
>> +
>>       po[OPT_ADVANCED_GROUP].title = SANE_I18N("Advanced");
>>       po[OPT_ADVANCED_GROUP].desc = "";
>>       po[OPT_ADVANCED_GROUP].type = SANE_TYPE_GROUP;
>> @@ -942,7 +1049,16 @@ static void control_options(SnapScan_Scanner *pss)
>>           default:
>>               break;
>>           }
>> -    }
>> +    }
>> +    if (pss->pdev->model == SCANWIT2720S)
>> +    {
>> +        pss->options[OPT_FRAME_NO].cap &= ~SANE_CAP_INACTIVE;
>> +        pss->options[OPT_FOCUS_MODE].cap &= ~SANE_CAP_INACTIVE;
>> +        if (pss->focus_mode == MD_MANUAL)
>> +        {
>> +            pss->options[OPT_FOCUS_POINT].cap &= ~SANE_CAP_INACTIVE;
>> +        }
>> +    }
>>   }
>>
>>   SANE_Status sane_control_option (SANE_Handle h,
>> @@ -1080,7 +1196,16 @@ SANE_Status sane_control_option (SANE_Handle h,
>>               break;
>>           case OPT_BIT_DEPTH:
>>               *(SANE_Int *) v = pss->val[OPT_BIT_DEPTH].w;
>> -            break;
>> +            break;
>> +        case OPT_FRAME_NO:
>> +            *(SANE_Int *) v = pss->frame_no;
>> +            break;
>> +        case OPT_FOCUS_MODE:
>> +            strcpy ((SANE_String) v, pss->focus_mode_s);
>> +            break;
>> +        case OPT_FOCUS_POINT:
>> +            *(SANE_Int *) v = pss->focus;
>> +            break;
>>           default:
>>               DBG (DL_MAJOR_ERROR,
>>                    "%s: invalid option number %ld\n",
>> @@ -1495,6 +1620,31 @@ SANE_Status sane_control_option (SANE_Handle h,
>>               if (i)
>>                   *i |= SANE_INFO_RELOAD_PARAMS;
>>               break;
>> +        case OPT_FRAME_NO:
>> +            pss->frame_no = *(SANE_Int *) v;
>> +            break;
>> +        case OPT_FOCUS_MODE:
>> +            {
>> +                char *s = (SANE_String) v;
>> +                if (strcmp (s, md_manual) == 0)
>> +                {
>> +                    pss->focus_mode_s = md_manual;
>> +                    pss->focus_mode = MD_MANUAL;
>> +                    pss->options[OPT_FOCUS_POINT].cap &= ~SANE_CAP_INACTIVE;
>> +                }
>> +                else
>> +                {
>> +                    pss->focus_mode_s = md_auto;
>> +                    pss->focus_mode = MD_AUTO;
>> +                    pss->options[OPT_FOCUS_POINT].cap |= SANE_CAP_INACTIVE;
>> +                }
>> +                if (i)
>> +                    *i = SANE_INFO_RELOAD_OPTIONS | SANE_INFO_RELOAD_PARAMS;
>> +                break;
>> +            }
>> +        case OPT_FOCUS_POINT:
>> +            pss->focus = *(SANE_Int *) v;
>> +            break;
>>           default:
>>               DBG (DL_MAJOR_ERROR,
>>                    "%s: invalid option number %ld\n",
>> @@ -1526,7 +1676,14 @@ SANE_Status sane_control_option (SANE_Handle h,
>>           switch (n)
>>           {
>>           case OPT_SCANRES:
>> -            pss->res = 300;
>> +            if (pss->pdev->model == SCANWIT2720S)
>> +            {
>> +                pss->res = 1350;
>
> So as the 2720S is a film scanner, it gets a higher default(?)
> resolution?

300 is not a possible resolution for the 2720S so it had to have 
something different anyway. Anything much smaller than 1200 for a film 
scanner is of little use, IMO, and 1350 is the closest to 1200 for the 
2720S.
But basically yes, a film scanner warrants a higher default resolution.

>> +            }
>> +            else
>> +            {
>> +                pss->res = 300;
>> +            }
>>               if (i)
>>                   *i |= SANE_INFO_RELOAD_PARAMS;
>>               break;
>> @@ -1558,13 +1715,26 @@ SANE_Status sane_control_option (SANE_Handle h,
>>               pss->preview_mode = MD_GREYSCALE;
>>               break;
>>           case OPT_SOURCE:
>> -            pss->source = SRC_FLATBED;
>> -            pss->pdev->x_range.max = x_range_fb.max;
>> -            pss->pdev->y_range.max = y_range_fb.max;
>> -            pss->predef_window = pdw_none;
>> -            if (pss->source_s)
>> -                free (pss->source_s);
>> -            pss->source_s = (SANE_Char *) strdup(src_flatbed);
>> +            if (pss->pdev->model == SCANWIT2720S)
>> +            {
>> +                pss->source = SRC_TPO;
>> +                pss->pdev->x_range.max = x_range_tpo.max;
>> +                pss->pdev->y_range.max = y_range_tpo.max;
>> +                pss->predef_window = pdw_none;
>> +                if (pss->source_s)
>> +                    free (pss->source_s);
>> +                pss->source_s = (SANE_Char *) strdup(src_tpo);
>> +            }
>> +            else
>> +            {
>> +                pss->source = SRC_FLATBED;
>> +                pss->pdev->x_range.max = x_range_fb.max;
>> +                pss->pdev->y_range.max = y_range_fb.max;
>> +                pss->predef_window = pdw_none;
>> +                if (pss->source_s)
>> +                    free (pss->source_s);
>> +                pss->source_s = (SANE_Char *) strdup(src_flatbed);
>> +            }
>>               if (i)
>>                   *i |= SANE_INFO_RELOAD_PARAMS | SANE_INFO_RELOAD_OPTIONS;
>>               break;
>> @@ -1598,7 +1768,26 @@ SANE_Status sane_control_option (SANE_Handle h,
>>               pss->gs_lpr = def_gs_lpr;
>>               break;
>>           case OPT_BIT_DEPTH:
>> -            pss->val[OPT_BIT_DEPTH].w = def_bpp;
>> +            if (pss->pdev->model == SCANWIT2720S)
>> +            {
>> +                pss->val[OPT_BIT_DEPTH].w = 12;
>> +            }
>> +            else
>> +            {
>> +                pss->val[OPT_BIT_DEPTH].w = def_bpp;
>> +            }
>> +            break;
>> +        case OPT_FRAME_NO:
>> +            pss->frame_no = def_frame_no;
>> +            break;
>> +        case OPT_FOCUS_MODE:
>> +            pss->focus_mode_s = md_auto;
>> +            pss->focus_mode = MD_AUTO;
>> +            if (i)
>> +                *i = SANE_INFO_RELOAD_OPTIONS;
>> +            break;
>> +        case OPT_FOCUS_POINT:
>> +            pss->focus = 0x13e;
>
> What does 0x13e imply?

It is just an arbitrary starting point to use if manual focus mode is 
selected. It happens to be a good enough focus point for my scanner. I 
expect it to be close on most scanners. It will make selecting the 
desired focus point quicker than starting from either extreme.

>>               break;
>>           default:
>>               DBG (DL_MAJOR_ERROR,
>> diff --git a/backend/snapscan-scsi.c b/backend/snapscan-scsi.c
>> index 7482215..3b0c697 100644
>> --- a/backend/snapscan-scsi.c
>> +++ b/backend/snapscan-scsi.c
>> @@ -1,9 +1,9 @@
>>   /* sane - Scanner Access Now Easy.
>>
>> -   Copyright (C) 1997, 1998, 2001 Franck Schnefra, Michel Roelofs,
>> +   Copyright (C) 1997, 1998, 2001, 2002, 2013 Franck Schnefra, Michel Roelofs,
>>      Emmanuel Blot, Mikko Tyolajarvi, David Mosberger-Tang, Wolfgang Goeller,
>>      Petter Reinholdtsen, Gary Plewa, Sebastien Sable, Mikael Magnusson,
>> -   Oliver Schwartz and Kevin Charter
>> +   Max Ushakov, Andrew Goodbody, Oliver Schwartz and Kevin Charter
>>
>>      This file is part of the SANE package.
>>
>> @@ -289,6 +289,7 @@ static SANE_Status snapscan_cmd(SnapScan_Bus bus, int fd, const void *src,
>>   #define RESERVE_UNIT           0x16
>>   #define RELEASE_UNIT           0x17
>>   #define SEND_DIAGNOSTIC        0x1D
>> +#define OBJECT_POSITION        0x31
>>   #define GET_DATA_BUFFER_STATUS 0x34
>>
>>   #define SCAN_LEN 6
>> @@ -561,6 +562,7 @@ static SANE_Status inquiry (SnapScan_Scanner *pss)
>>           pss->bpp = 14;
>>           break;
>>       case STYLUS_CX1500:
>> +    case SCANWIT2720S:
>>           pss->bpp = 12;
>>           break;
>>       default:
>> @@ -674,6 +676,10 @@ static void release_unit (SnapScan_Scanner *pss)
>>   #define DTCQ_GAMMA_RED14 0x96
>>   #define DTCQ_GAMMA_GREEN14 0x97
>>   #define DTCQ_GAMMA_BLUE14 0x98
>> +#define DTCQ_GAMMA_GRAY12_16BIT 0xa0
>> +#define DTCQ_GAMMA_RED12_16BIT 0xa1
>> +#define DTCQ_GAMMA_GREEN12_16BIT 0xa2
>> +#define DTCQ_GAMMA_BLUE12_16BIT 0xa3
>>   #define DTCQ_GAMMA_GRAY14_16BIT 0xa5 /* ? */
>>   #define DTCQ_GAMMA_RED14_16BIT 0xa6
>>   #define DTCQ_GAMMA_GREEN14_16BIT 0xa7
>> @@ -734,6 +740,12 @@ static SANE_Status send (SnapScan_Scanner *pss, u_char dtc, u_char dtcq)
>>           case DTCQ_GAMMA_BLUE12:
>>               tl = 4096;
>>               break;
>> +        case DTCQ_GAMMA_GRAY12_16BIT:    /* 12-bit tables with 16 bit data */
>> +        case DTCQ_GAMMA_RED12_16BIT:
>> +        case DTCQ_GAMMA_GREEN12_16BIT:
>> +        case DTCQ_GAMMA_BLUE12_16BIT:
>> +            tl = 8192;
>> +            break;
>>           case DTCQ_GAMMA_GRAY14:    /* 14-bit tables */
>>           case DTCQ_GAMMA_RED14:
>>           case DTCQ_GAMMA_GREEN14:
>> @@ -832,13 +844,10 @@ static SANE_Status send_calibration_5150(SnapScan_Scanner *pss)
>>   #define SET_WINDOW_P_BLUE_UNDER_COLOR      45
>>   #define SET_WINDOW_P_GREEN_UNDER_COLOR     44
>>
>> -static SANE_Status set_window (SnapScan_Scanner *pss)
>> +static SANE_Status prepare_set_window (SnapScan_Scanner *pss)
>>   {
>> -    static const char *me = "set_window";
>> -    SANE_Status status;
>> -    unsigned char source;
>> +    static const char *me = "prepare_set_window";
>>       u_char *pc;
>> -    int pos_factor;
>>
>>       DBG (DL_CALL_TRACE, "%s\n", me);
>>       zero_buf (pss->cmd, MAX_SCSI_CMD_LEN);
>> @@ -860,62 +869,11 @@ static SANE_Status set_window (SnapScan_Scanner *pss)
>>       u_short_to_u_charp (pss->res, pc + SET_WINDOW_P_YRES);
>>       DBG (DL_CALL_TRACE, "%s Resolution: %d\n", me, pss->res);
>>
>> -    switch (pss->pdev->model)
>> -    {
>> -        case PRISA5000:
>> -        case PRISA5000E:
>> -        case PRISA5150:
>> -            pos_factor = (pss->res > 600) ?  1200 : 600;
>> -            break;
>> -        case PERFECTION1270:
>> -        case PERFECTION1670:
>> -            pos_factor = (pss->res > 800) ?  1600 : 800;
>> -            break;
>> -        case PERFECTION2480:
>> -            pos_factor = (pss->res > 1200) ?  2400 : 1200;
>> -            break;
>> -        case PERFECTION3490:
>> -            pos_factor = (pss->res > 1600) ?  3200 : 1600;
>> -            break;
>> -        default:
>> -            pos_factor = pss->actual_res;
>> -            break;
>> -    }
>> -    /* it's an ugly sound if the scanner drives against the rear
>> -       wall, and with changing max values we better be sure */
>> -    check_range(&(pss->brx), pss->pdev->x_range);
>> -    check_range(&(pss->bry), pss->pdev->y_range);
>> -    {
>> -        int tlxp =
>> -            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->tlx));
>> -        int tlyp =
>> -            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->tly));
>> -        int brxp =
>> -            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->brx));
>> -        int bryp =
>> -            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->bry));
>> -
>> -        /* Check for brx > tlx and bry > tly */
>> -        if (brxp <= tlxp) {
>> -            tlxp = MAX (0, brxp - 75);
>> -        }
>> -        if (bryp <= tlyp) {
>> -            tlyp = MAX (0, bryp - 75);
>> -        }
>> -
>> -        u_int_to_u_char4p (tlxp, pc + SET_WINDOW_P_TLX);
>> -        u_int_to_u_char4p (tlyp, pc + SET_WINDOW_P_TLY);
>> -        u_int_to_u_char4p (MAX (((unsigned) (brxp - tlxp)), 75),
>> -                           pc + SET_WINDOW_P_WIDTH);
>> -        u_int_to_u_char4p (MAX (((unsigned) (bryp - tlyp)), 75),
>> -                           pc + SET_WINDOW_P_LENGTH);
>> -        DBG (DL_INFO, "%s Width:  %d\n", me, brxp-tlxp);
>> -        DBG (DL_INFO, "%s Length: %d\n", me, bryp-tlyp);
>> -    }
>>       pc[SET_WINDOW_P_BRIGHTNESS] = 128;
>>       pc[SET_WINDOW_P_THRESHOLD] =
>>           (u_char) (255.0*(pss->threshold / 100.0));
>>       pc[SET_WINDOW_P_CONTRAST] = 128;
>> +
>>       {
>>           SnapScan_Mode mode = pss->mode;
>>           pss->bpp_scan = pss->val[OPT_BIT_DEPTH].w;
>> @@ -923,7 +881,8 @@ static SANE_Status set_window (SnapScan_Scanner *pss)
>>           if (pss->preview)
>>           {
>>               mode = pss->preview_mode;
>> -            pss->bpp_scan = 8;
>> +            if (pss->pdev->model != SCANWIT2720S)
>> +                pss->bpp_scan = 8;
>
> Should there be an else branch?

No. bpp_scan was already set to the correct value for the 2720S in 
init_options (see above). This code may not be needed at all here with 
the above change but I did not want to rock the boat too much and upset 
other scanners that I could not test.

>>           }
>>
>>           DBG (DL_MINOR_INFO, "%s Mode: %d\n", me, mode);
>> @@ -982,6 +941,87 @@ static SANE_Status set_window (SnapScan_Scanner *pss)
>>               pc[SET_WINDOW_P_GAMMA_NO] = 0x01;   /* downloaded gamma table */
>>           }
>>       }
>> +
>> +    pc[SET_WINDOW_P_RED_UNDER_COLOR] = 0xff;    /* defaults */
>> +    pc[SET_WINDOW_P_BLUE_UNDER_COLOR] = 0xff;
>> +    pc[SET_WINDOW_P_GREEN_UNDER_COLOR] = 0xff;
>> +
>> +    return SANE_STATUS_GOOD;
>> +}
>> +
>> +static SANE_Status set_window (SnapScan_Scanner *pss)
>> +{
>> +    static const char *me = "set_window";
>> +    SANE_Status status;
>> +    unsigned char source;
>> +    u_char *pc;
>> +    int pos_factor;
>> +
>> +    DBG (DL_CALL_TRACE, "%s\n", me);
>> +    status = prepare_set_window(pss);
>> +    CHECK_STATUS (status, me, "prepare_set_window");
>> +
>> +    pc = pss->cmd;
>> +
>> +    /* header; we support only one window */
>> +    pc += SET_WINDOW_LEN;
>> +
>> +    /* the sole window descriptor */
>> +    pc += SET_WINDOW_HEADER_LEN;
>> +
>> +    switch (pss->pdev->model)
>> +    {
>> +        case PRISA5000:
>> +        case PRISA5000E:
>> +        case PRISA5150:
>> +            pos_factor = (pss->res > 600) ?  1200 : 600;
>> +            break;
>> +        case PERFECTION1270:
>> +        case PERFECTION1670:
>> +            pos_factor = (pss->res > 800) ?  1600 : 800;
>> +            break;
>> +        case PERFECTION2480:
>> +            pos_factor = (pss->res > 1200) ?  2400 : 1200;
>> +            break;
>> +        case PERFECTION3490:
>> +            pos_factor = (pss->res > 1600) ?  3200 : 1600;
>> +            break;
>> +        default:
>> +            pos_factor = pss->actual_res;
>> +            break;
>> +    }
>> +    /* it's an ugly sound if the scanner drives against the rear
>> +       wall, and with changing max values we better be sure */
>> +    check_range(&(pss->brx), pss->pdev->x_range);
>> +    check_range(&(pss->bry), pss->pdev->y_range);
>> +    {
>> +        int tlxp =
>> +            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->tlx));
>> +        int tlyp =
>> +            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->tly));
>> +        int brxp =
>> +            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->brx));
>> +        int bryp =
>> +            (int) (pos_factor*IN_PER_MM*SANE_UNFIX(pss->bry));
>> +
>> +        /* Check for brx > tlx and bry > tly */
>> +        if (brxp <= tlxp) {
>> +            tlxp = MAX (0, brxp - 75);
>> +        }
>> +        if (bryp <= tlyp) {
>> +            tlyp = MAX (0, bryp - 75);
>> +        }
>> +
>> +        u_int_to_u_char4p (tlxp, pc + SET_WINDOW_P_TLX);
>> +        u_int_to_u_char4p (tlyp, pc + SET_WINDOW_P_TLY);
>> +        u_int_to_u_char4p (MAX (((unsigned) (brxp - tlxp)), 75),
>> +                           pc + SET_WINDOW_P_WIDTH);
>> +        u_int_to_u_char4p (MAX (((unsigned) (bryp - tlyp)), 75),
>> +                           pc + SET_WINDOW_P_LENGTH);
>> +        DBG (DL_INFO, "%s Width:  %d\n", me, brxp-tlxp);
>> +        DBG (DL_INFO, "%s Length: %d\n", me, bryp-tlyp);
>> +    }
>> +
>>       source = 0x0;
>>       if (pss->preview) {
>>           source |= 0x80; /* no high quality in preview */
>> @@ -1003,9 +1043,6 @@ static SANE_Status set_window (SnapScan_Scanner *pss)
>>       }
>>       pc[SET_WINDOW_P_OPERATION_MODE] = source;
>>       DBG (DL_MINOR_INFO, "%s: operation mode set to 0x%02x\n", me, (int) source);
>> -    pc[SET_WINDOW_P_RED_UNDER_COLOR] = 0xff;    /* defaults */
>> -    pc[SET_WINDOW_P_BLUE_UNDER_COLOR] = 0xff;
>> -    pc[SET_WINDOW_P_GREEN_UNDER_COLOR] = 0xff;
>>
>>       do {
>>           status = snapscan_cmd (pss->pdev->bus, pss->fd, pss->cmd,
>> @@ -1020,6 +1057,91 @@ static SANE_Status set_window (SnapScan_Scanner *pss)
>>       return status;
>>   }
>>
>> +static SANE_Status set_window_autofocus (SnapScan_Scanner *copy)
>> +{
>> +    static char me[] = "set_window_autofocus";
>> +    SANE_Status status;
>> +
>> +    DBG (DL_CALL_TRACE, "%s(%p)\n", me, (void*)copy);
>> +
>> +    copy->res = copy->actual_res;
>> +    status = prepare_set_window (copy);
>> +    CHECK_STATUS (status, me, "prepare_set_window");
>
> • set_window_autofocus
> • Could __func__ or something like this be used?

This is just how it is done in other functions. Some functions do not 
use the actual function name as me[].

>> +
>> +    u_int_to_u_char4p (1700, copy->cmd + SET_WINDOW_DESC + SET_WINDOW_P_TLY);
>> +    /* fill in width & height */
>> +    u_int_to_u_char4p (2550, copy->cmd + SET_WINDOW_DESC + SET_WINDOW_P_WIDTH);
>> +    u_int_to_u_char4p (128, copy->cmd + SET_WINDOW_DESC + SET_WINDOW_P_LENGTH);
>> +
>> +    copy->cmd[SET_WINDOW_DESC + SET_WINDOW_P_BITS_PER_PIX] = 12;
>> +    copy->cmd[SET_WINDOW_DESC + SET_WINDOW_P_OPERATION_MODE] = 0x49; /* focusing mode */
>> +    return snapscan_cmd (copy->pdev->bus, copy->fd, copy->cmd,
>> +                SET_WINDOW_TOTAL_LEN, NULL, NULL);
>> +}
>> +
>> +#define SET_FRAME_LEN  10
>> +
>> +static SANE_Status set_frame (SnapScan_Scanner *pss, SANE_Byte frame_no)
>> +{
>> +    static const char *me = "set_frame";
>> +    SANE_Status status;
>> +
>> +    DBG (DL_CALL_TRACE, "%s\n", me);
>> +    DBG (DL_VERBOSE, "%s setting frame to %d\n", me, frame_no);
>> +    zero_buf (pss->cmd, MAX_SCSI_CMD_LEN);
>> +    pss->cmd[0] = OBJECT_POSITION;
>> +    pss->cmd[1] = 2;          /* Absolute position used here to select the frame */
>> +    pss->cmd[4] = frame_no;
>> +
>> +    status = snapscan_cmd (pss->pdev->bus, pss->fd, pss->cmd, SET_FRAME_LEN, NULL, NULL);
>> +    CHECK_STATUS (status, me, "snapscan_cmd");
>> +
>> +    return status;
>> +}
>> +
>> +static SANE_Status set_focus (SnapScan_Scanner *pss, SANE_Int focus)
>> +{
>> +    static const char *me = "set_focus";
>> +    SANE_Status status;
>> +
>> +    DBG (DL_CALL_TRACE, "%s(%d)\n", me, focus);
>> +    zero_buf (pss->cmd, MAX_SCSI_CMD_LEN);
>> +    pss->cmd[0] = OBJECT_POSITION;
>> +    pss->cmd[1] = 4;          /* Rotate object but here it sets the focus point */
>> +    pss->cmd[3] = (focus >> 8) & 0xFF;
>> +    pss->cmd[4] = focus & 0xFF;
>> +    status = snapscan_cmd (pss->pdev->bus, pss->fd, pss->cmd, SET_FRAME_LEN, NULL, NULL);
>> +    CHECK_STATUS (status, me, "snapscan_cmd");
>> +    return status;
>> +}
>> +
>> +static SANE_Int get_8 (u_char *p)
>> +{
>> +    SANE_Int b;
>> +    b = p[0] | (p[1] << 8);
>> +    return b;
>> +}
>> +
>> +
>
> Only one empty line?

Done.

>> +static double get_val (u_char *p, SANE_Int len, SANE_Int x)
>> +{
>> +       return get_8 (p + ((x + len) << 1)) / 255.0;
>> +}
>> +
>> +static double func (u_char *p, int len)
>
> Maybe a more descriptive name can be chosen here?

Yes, but what? See below.

>> +{
>> +       double v, m, s;
>> +       SANE_Int i;
>> +
>> +       s = 0;
>> +       for (i = 0; i < len-1; i++) {
>> +               v = get_val (p, len, i);
>> +               m = get_val (p, len, i+1);
>> +               s += fabs (v - m);
>> +       }
>> +       return s;
>> +}
>> +
>>   static SANE_Status scan (SnapScan_Scanner *pss)
>>   {
>>       static const char *me = "scan";
>> @@ -1061,6 +1183,60 @@ static SANE_Status scsi_read (SnapScan_Scanner *pss, u_char read_type)
>>       return status;
>>   }
>>
>> +static SANE_Status get_focus (SnapScan_Scanner *pss)
>> +{
>> +    static const char *me = "get_focus";
>> +    SANE_Int f, max_f;
>
> More descriptive names? What does f mean? focus?

Changed to focus_point and max_focus_point.

>> +    double sum, max;
>> +    SANE_Status status;
>> +    SnapScan_Scanner copy;
>> +
>> +    copy = *pss;
>
> Why not pass *pss around?

Because some internal values are changed and this removes the need to 
save and revert them.

>> +
>> +    DBG (DL_CALL_TRACE, "%s\n", me);
>> +    reserve_unit(&copy);
>> +
>> +    status = set_window_autofocus (&copy);
>> +    CHECK_STATUS (status, me, "set_window_autofocus");
>> +
>> +    status = inquiry (&copy);
>> +    CHECK_STATUS (status, me, "inquiry");
>> +
>> +    status = scan (&copy);
>> +    CHECK_STATUS (status, me, "scan");
>> +
>> +    status = set_frame (&copy, copy.frame_no);
>> +    CHECK_STATUS (status, me, "set_frame2");
>
> No 2 at end.

Done.

>> +
>> +    DBG (DL_VERBOSE, "%s: Expected number of bytes for each read %d\n", me, (int)copy.bytes_per_line);
>> +    DBG (DL_VERBOSE, "%s: Expected number of pixels per line %d\n", me, (int)copy.pixels_per_line);
>
> What is done in the following loop?

The scanner has been put into a special mode for performing the 
focussing. The data it returns is not usual image data. The algorithm in 
func above is applied to this focussing data. When the result of that 
algorithm peaks that current focus_point is recorded as the point of 
best focus. I do not know the form of the data returned in this 
focussing mode. This code came from Max Ushakov.
My guess is that in focussing mode the scanner only returns alpha 
channel data. func seems to be summing the differences from pixel to 
pixel. When this value peaks you will have achieved the maximum contrast 
and hence best focus. So maybe func should be renamed sum_pixel_differences.

>> +    max_f = -1;
>> +    max = -1;
>> +    for (f = 0; f <= 0x300; f += 6) {
>> +        status = set_focus (&copy, f);
>> +        CHECK_STATUS (status, me, "set_focus");
>> +
>> +        copy.expected_read_bytes = copy.bytes_per_line;
>> +        status = scsi_read (&copy, READ_IMAGE);
>> +        CHECK_STATUS (status, me, "scsi_read");
>> +
>> +        sum = func (copy.buf, copy.pixels_per_line);
>> +
>> +        if (sum > max) {
>> +            max = sum;
>> +            max_f = f;
>> +        }
>> +    }
>> +    pss->focus = max_f;
>> +    DBG (DL_VERBOSE, "%s: Focus point found to be at 0x%x\n", me, max_f);
>> +    release_unit (&copy);
>> +
>> +    wait_scanner_ready (&copy);
>> +    CHECK_STATUS (status, me, "wait_scanner_ready");
>> +
>> +    return status;
>> +}
>> +
>>   /*
>>   static SANE_Status request_sense (SnapScan_Scanner *pss)
>>   {
>> @@ -1100,6 +1276,8 @@ static SANE_Status send_diagnostic (SnapScan_Scanner *pss)
>>           ||
>>        pss->pdev->model == SNAPSCAN1236
>>           ||
>> +     pss->pdev->model == SCANWIT2720S
>> +        ||
>>           pss->pdev->model == ARCUS1200)
>>       {
>>           return SANE_STATUS_GOOD;
>> @@ -1378,6 +1556,8 @@ static SANE_Status calibrate (SnapScan_Scanner *pss)
>>       {
>>          return send_calibration_5150(pss);
>>       }
>> +
>> +    DBG (DL_CALL_TRACE, "%s\n", me);
>>
>>       if (line_length) {
>>           int num_lines = pss->phys_buf_sz / line_length;
>> @@ -1425,7 +1605,6 @@ static SANE_Status download_firmware(SnapScan_Scanner * pss)
>>       SANE_Status status = SANE_STATUS_GOOD;
>>       char cModelName[8], cModel[255];
>>       unsigned char bModelNo;
>> -    int readByte;
>>
>>       bModelNo =*(pss->buf + INQUIRY_HWMI);
>>       zero_buf((unsigned char *)cModel, 255);
>> @@ -1512,7 +1691,7 @@ static SANE_Status download_firmware(SnapScan_Scanner * pss)
>>               pCDB = (unsigned char *)malloc(bufLength + cdbLength);
>>               pFwBuf = pCDB + cdbLength;
>>               zero_buf (pCDB, cdbLength);
>> -            readByte = fread(pFwBuf,1,bufLength,fd);
>> +            (void)fread(pFwBuf,1,bufLength,fd);
>
> Why?

To avoid a compiler warning about readByte being assigned but then unused.

>>
>>               *pCDB = SEND;
>>               *(pCDB + 2) = 0x87;
>> diff --git a/backend/snapscan-sources.c b/backend/snapscan-sources.c
>> index bd15e8e..d91060e 100644
>> --- a/backend/snapscan-sources.c
>> +++ b/backend/snapscan-sources.c
>> @@ -1,9 +1,9 @@
>>   /* sane - Scanner Access Now Easy.
>>
>> -   Copyright (C) 1997, 1998 Franck Schnefra, Michel Roelofs,
>> +   Copyright (C) 1997, 1998, 2002, 2013 Franck Schnefra, Michel Roelofs,
>>      Emmanuel Blot, Mikko Tyolajarvi, David Mosberger-Tang, Wolfgang Goeller,
>> -   Petter Reinholdtsen, Gary Plewa, Sebastien Sable, Oliver Schwartz
>> -   and Kevin Charter
>> +   Petter Reinholdtsen, Gary Plewa, Sebastien Sable, Max Ushakov,
>> +   Andrew Goodbody, Oliver Schwartz and Kevin Charter
>>
>>      This file is part of the SANE package.
>>
>> @@ -932,6 +932,13 @@ typedef struct
>>       SANE_Int round_read;
>>   } RGBRouter;
>>
>> +static void put_int16r (int n, u_char *p)
>> +{
>> +       p[0] = (n & 0x00ff);
>> +       p[1] = (n & 0xff00) >> 8;
>> +}
>> +
>> +
>>   static SANE_Int RGBRouter_remaining (Source *pself)
>>   {
>>       RGBRouter *ps = (RGBRouter *) pself;
>> @@ -951,7 +958,7 @@ static SANE_Status RGBRouter_get (Source *pself,
>>       SANE_Status status = SANE_STATUS_GOOD;
>>       SANE_Int remaining = *plen;
>>       SANE_Byte *s;
>> -    SANE_Int i;
>> +    SANE_Int i, t;
>>       SANE_Int r, g, b;
>>       SANE_Int run_req;
>>       SANE_Int org_len = *plen;
>> @@ -998,6 +1005,22 @@ static SANE_Status RGBRouter_get (Source *pself,
>>                       *s++ = ps->cbuf[g++];
>>                       *s++ = ps->cbuf[b++];
>>                   }
>> +                else if (pself->pss->pdev->model == SCANWIT2720S)
>> +                {
>> +                    t = (((ps->cbuf[r+1] << 8) | ps->cbuf[r]) & 0xfff) << 4;
>> +                    put_int16r (t, s);
>> +                    s += 2;
>> +                    r += 2;
>> +                    t = (((ps->cbuf[g+1] << 8) | ps->cbuf[g]) & 0xfff) << 4;
>> +                    put_int16r (t, s);
>> +                    s += 2;
>> +                    g += 2;
>> +                    t = (((ps->cbuf[b+1] << 8) | ps->cbuf[b]) & 0xfff) << 4;
>> +                    put_int16r (t, s);
>> +                    s += 2;
>> +                    b += 2;
>> +                    i++;
>> +                }
>>                   else
>>                   {
>>                       *s++ = ps->cbuf[r++];
>> diff --git a/backend/snapscan.c b/backend/snapscan.c
>> index 284ad08..7885dee 100644
>> --- a/backend/snapscan.c
>> +++ b/backend/snapscan.c
>> @@ -1,9 +1,10 @@
>>   /* sane - Scanner Access Now Easy.
>>
>> -   Copyright (C) 1997-2005 Franck Schnefra, Michel Roelofs, Emmanuel Blot,
>> -   Mikko Tyolajarvi, David Mosberger-Tang, Wolfgang Goeller, Simon Munton,
>> -   Petter Reinholdtsen, Gary Plewa, Sebastien Sable, Mikael Magnusson,
>> -   Oliver Schwartz and Kevin Charter
>> +   Copyright (C) 1997-2005, 2013 Franck Schnefra, Michel Roelofs,
>> +   Emmanuel Blot, Mikko Tyolajarvi, David Mosberger-Tang, Wolfgang Goeller,
>> +   Simon Munton, Petter Reinholdtsen, Gary Plewa, Sebastien Sable,
>> +   Mikael Magnusson, Max Ushakov, Andrew Goodbody, Oliver Schwartz
>> +   and Kevin Charter
>>
>>      This file is part of the SANE package.
>>
>> @@ -133,6 +134,10 @@ if ((s) != SANE_STATUS_GOOD) { DBG(DL_MAJOR_ERROR, "%s: %s command failed: %s\n"
>>   #define MM_PER_IN 25.4                /* # millimetres per inch */
>>   #define IN_PER_MM 0.03937        /* # inches per millimetre  */
>>
>> +#define GAMMA_8BIT     0
>> +#define GAMMA_16BIT    1
>> +#define GAMMA_12_16BIT 2
>> +
>>   #ifndef SANE_I18N
>>   #define SANE_I18N(text) text
>>   #endif
>> @@ -147,7 +152,7 @@ static SANE_Char password[SANE_MAX_PASSWORD_LEN];
>>   /* function prototypes */
>>
>>   static void gamma_n (double gamma, int brightness, int contrast,
>> -                     u_char *buf, int length, int gamma_16bit);
>> +                     u_char *buf, int length, int gamma_mode);
>>   static void gamma_to_sane (int length, u_char *in, SANE_Int *out);
>>
>>   static size_t max_string_size(SANE_String_Const strings[]);
>> @@ -181,11 +186,16 @@ static inline int calibration_line_length(SnapScan_Scanner *pss)
>>       case PERFECTION2480:
>>       case PERFECTION3490:
>>           pos_factor = pss->actual_res / 2;
>> +        pixel_length = pos_factor * 8.5;
>> +        break;
>> +    case SCANWIT2720S:
>> +        pixel_length = 2550;
>
> Can this be calculated for the 2720S too?

Yes possibly but there is no point. The generic calculations are for 
getting a value for an 8.5" line length, which is wrong for the 2720S. 
The calculations will always give the same value for any one particular 
scanner anyway. Creating a calculation for the 2720S just to give the 
desired value is simply pointless obfuscation.

>> +        break;
>>       default:
>>           pos_factor = pss->actual_res;
>> +        pixel_length = pos_factor * 8.5;
>>           break;
>>       }
>> -    pixel_length = pos_factor * 8.5;
>>
>>       if(is_colour_mode(actual_mode(pss))) {
>>           return 3 * pixel_length;
>> @@ -285,7 +295,7 @@ static size_t max_string_size (SANE_String_Const strings[])
>>
>>   /* gamma table computation */
>>   static void gamma_n (double gamma, int brightness, int contrast,
>> -                      u_char *buf, int bpp, int gamma_16bit)
>> +                      u_char *buf, int bpp, int gamma_mode)
>
> Ok, just renaming.

Renaming due to changing the use from a boolean.

>>   {
>>       int i;
>>       double i_gamma = 1.0/gamma;
>> @@ -295,27 +305,37 @@ static void gamma_n (double gamma, int brightness, int contrast,
>>
>>       for (i = 0;  i < length;  i++)
>>       {
>> +        int x;
>>           double val = (i - mid) * (1.0 + contrast / 100.0)
>>               + (1.0 + brightness / 100.0) * mid;
>>           val = LIMIT(val, 0, max);
>> -        if (gamma_16bit)
>> +        switch (gamma_mode)
>>           {
>> -            int x = LIMIT(65535*pow ((double) val/max, i_gamma) + 0.5, 0, 65535);
>> +        case GAMMA_16BIT:
>> +            x = LIMIT(65535*pow ((double) val/max, i_gamma) + 0.5, 0, 65535);
>>
>>               buf[2*i] = (u_char) x;
>>               buf[2*i + 1] = (u_char) (x >> 8);
>> -        }
>> -        else
>> +            break;
>> +        case GAMMA_12_16BIT:
>> +            buf[2*i] = (u_char) i;
>> +            buf[2*i + 1] = (u_char) (i >> 8);
>> +            break;
>> +        case GAMMA_8BIT:
>>               buf[i] =
>>                   (u_char) LIMIT(255*pow ((double) val/max, i_gamma) + 0.5, 0, 255);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>>       }
>>   }
>>
>> -static void gamma_from_sane (int length, SANE_Int *in, u_char *out, int gamma_16bit)
>> +static void gamma_from_sane (int length, SANE_Int *in, u_char *out, int gamma_mode)
>>   {
>>       int i;
>>       for (i = 0; i < length; i++)
>> -        if (gamma_16bit)
>> +        if (gamma_mode != GAMMA_8BIT)
>
> What bug does this fix?

The gamma calculations did not work for the 2720S. So instead of using 
as a boolean to simply choose between 8 and 16 bit gamma values, a third 
mode had to be allowed for. This third mode also used 16 bit gamma 
values so the test above had to change.

>>           {
>>               out[2*i] = (u_char) LIMIT(in[i], 0, 65535);
>>               out[2*i + 1] = (u_char) (LIMIT(in[i], 0, 65535) >> 8);
>> @@ -468,7 +488,14 @@ static SANE_Status snapscani_init_device_structure(
>>           (*pd)->dev.vendor = strdup (vendor);
>>       }
>>       (*pd)->dev.model = strdup (model);
>> -    (*pd)->dev.type = strdup (SNAPSCAN_TYPE);
>> +    if (model_num == SCANWIT2720S)
>> +    {
>> +        (*pd)->dev.type = strdup (SNAPSCAN_FS_TYPE);
>> +    }
>> +    else
>> +    {
>> +        (*pd)->dev.type = strdup (SNAPSCAN_TYPE);
>> +    }
>>       (*pd)->bus = bus_type;
>>       (*pd)->model = model_num;
>>
>> @@ -1111,6 +1138,8 @@ SANE_Status sane_get_parameters (SANE_Handle h,
>>       p->format = (is_colour_mode(mode)) ? SANE_FRAME_RGB : SANE_FRAME_GRAY;
>>       if (mode == MD_LINEART)
>>           p->depth = 1;
>> +    else if (pss->pdev->model == SCANWIT2720S)
>> +        p->depth = 16;
>>       else if (pss->preview)
>>           p->depth = 8;
>>       else
>> @@ -1333,7 +1362,7 @@ static SANE_Status download_gamma_tables (SnapScan_Scanner *pss)
>>       int dtcq_gamma_red;
>>       int dtcq_gamma_green;
>>       int dtcq_gamma_blue;
>> -    int gamma_16bit = 0;
>> +    int gamma_mode = GAMMA_8BIT;
>>
>>       DBG (DL_CALL_TRACE, "%s\n", me);
>>       switch (mode)
>> @@ -1367,10 +1396,22 @@ static SANE_Status download_gamma_tables (SnapScan_Scanner *pss)
>>           dtcq_gamma_blue = DTCQ_GAMMA_BLUE10;
>>           break;
>>       case 12:
>> -        dtcq_gamma_gray = DTCQ_GAMMA_GRAY12;
>> -        dtcq_gamma_red = DTCQ_GAMMA_RED12;
>> -        dtcq_gamma_green = DTCQ_GAMMA_GREEN12;
>> -        dtcq_gamma_blue = DTCQ_GAMMA_BLUE12;
>> +        if (pss->pdev->model == SCANWIT2720S)
>> +        {
>> +            DBG (DL_DATA_TRACE, "%s: Sending 16bit gamma table for %d bpp\n", me, pss->bpp);
>
> Also add that DBG output for non-2720S devices?

Done.

>> +            dtcq_gamma_gray = DTCQ_GAMMA_GRAY12_16BIT;
>> +            dtcq_gamma_red = DTCQ_GAMMA_RED12_16BIT;
>> +            dtcq_gamma_green = DTCQ_GAMMA_GREEN12_16BIT;
>> +            dtcq_gamma_blue = DTCQ_GAMMA_BLUE12_16BIT;
>> +            gamma_mode = GAMMA_12_16BIT;
>> +        }
>> +        else
>> +        {
>> +            dtcq_gamma_gray = DTCQ_GAMMA_GRAY12;
>> +            dtcq_gamma_red = DTCQ_GAMMA_RED12;
>> +            dtcq_gamma_green = DTCQ_GAMMA_GREEN12;
>> +            dtcq_gamma_blue = DTCQ_GAMMA_BLUE12;
>> +        }
>>           break;
>>       case 14:
>>           if (pss->bpp_scan == 16)
>> @@ -1379,7 +1420,7 @@ static SANE_Status download_gamma_tables (SnapScan_Scanner *pss)
>>               dtcq_gamma_red = DTCQ_GAMMA_RED14_16BIT;
>>               dtcq_gamma_green = DTCQ_GAMMA_GREEN14_16BIT;
>>               dtcq_gamma_blue = DTCQ_GAMMA_BLUE14_16BIT;
>> -            gamma_16bit = 1;
>> +            gamma_mode = GAMMA_16BIT;
>>           }
>>           else
>>           {
>> @@ -1405,34 +1446,34 @@ static SANE_Status download_gamma_tables (SnapScan_Scanner *pss)
>>               {
>>                   /* Use greyscale gamma for all rgb channels */
>>                   gamma_from_sane (pss->gamma_length, pss->gamma_table_gs,
>> -                                 pss->buf + SEND_LENGTH, gamma_16bit);
>> +                                 pss->buf + SEND_LENGTH, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_red);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_from_sane (pss->gamma_length, pss->gamma_table_gs,
>> -                                 pss->buf + SEND_LENGTH, gamma_16bit);
>> +                                 pss->buf + SEND_LENGTH, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_green);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_from_sane (pss->gamma_length, pss->gamma_table_gs,
>> -                                 pss->buf + SEND_LENGTH, gamma_16bit);
>> +                                 pss->buf + SEND_LENGTH, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_blue);
>>                   CHECK_STATUS (status, me, "send");
>>               }
>>               else
>>               {
>>                   gamma_from_sane (pss->gamma_length, pss->gamma_table_r,
>> -                                 pss->buf + SEND_LENGTH, gamma_16bit);
>> +                                 pss->buf + SEND_LENGTH, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_red);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_from_sane (pss->gamma_length, pss->gamma_table_g,
>> -                                 pss->buf + SEND_LENGTH, gamma_16bit);
>> +                                 pss->buf + SEND_LENGTH, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_green);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_from_sane (pss->gamma_length, pss->gamma_table_b,
>> -                                 pss->buf + SEND_LENGTH, gamma_16bit);
>> +                                 pss->buf + SEND_LENGTH, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_blue);
>>                   CHECK_STATUS (status, me, "send");
>>               }
>> @@ -1443,34 +1484,34 @@ static SANE_Status download_gamma_tables (SnapScan_Scanner *pss)
>>               {
>>                   /* Use greyscale gamma for all rgb channels */
>>                   gamma_n (gamma_gs, pss->bright, pss->contrast,
>> -                         pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                         pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_red);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_n (gamma_gs, pss->bright, pss->contrast,
>> -                         pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                         pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_green);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_n (gamma_gs, pss->bright, pss->contrast,
>> -                         pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                         pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_blue);
>>                   CHECK_STATUS (status, me, "send");
>>               }
>>               else
>>               {
>>                   gamma_n (gamma_r, pss->bright, pss->contrast,
>> -                         pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                         pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_red);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_n (gamma_g, pss->bright, pss->contrast,
>> -                         pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                         pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_green);
>>                   CHECK_STATUS (status, me, "send");
>>
>>                   gamma_n (gamma_b, pss->bright, pss->contrast,
>> -                         pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                         pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>                   status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_blue);
>>                   CHECK_STATUS (status, me, "send");
>>               }
>> @@ -1481,14 +1522,14 @@ static SANE_Status download_gamma_tables (SnapScan_Scanner *pss)
>>           if(pss->val[OPT_CUSTOM_GAMMA].b)
>>           {
>>               gamma_from_sane (pss->gamma_length, pss->gamma_table_gs,
>> -                             pss->buf + SEND_LENGTH, gamma_16bit);
>> +                             pss->buf + SEND_LENGTH, gamma_mode);
>>               status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_gray);
>>               CHECK_STATUS (status, me, "send");
>>           }
>>           else
>>           {
>>               gamma_n (gamma_gs, pss->bright, pss->contrast,
>> -                     pss->buf + SEND_LENGTH, pss->bpp, gamma_16bit);
>> +                     pss->buf + SEND_LENGTH, pss->bpp, gamma_mode);
>>               status = send_gamma_table(pss, DTC_GAMMA, dtcq_gamma_gray);
>>               CHECK_STATUS (status, me, "send");
>>           }
>> @@ -1631,8 +1672,26 @@ SANE_Status sane_start (SANE_Handle h)
>>
>>       pss->state = ST_SCAN_INIT;
>>
>> +    if ((pss->pdev->model == SCANWIT2720S) && (pss->focus_mode == MD_AUTO))
>> +    {
>> +        status = get_focus(pss);
>> +        CHECK_STATUS (status, me, "get_focus");
>> +    }
>> +
>>       reserve_unit(pss);
>>
>> +    if (pss->pdev->model == SCANWIT2720S)
>> +    {
>> +        status = set_frame(pss, 0);
>> +        CHECK_STATUS (status, me, "set_frame1");
>
> Ah that is why the 2 from earlier. Maybe make that more clear though.
> First call or something like this.

Just removed it, as above, context and me[] should make clear which call 
is which.

>> +    }
>> +
>> +    if (pss->pdev->model == SCANWIT2720S)
>> +    {
>> +        status = set_focus(pss, pss->focus);
>> +        CHECK_STATUS (status, me, "set_focus");
>> +    }
>> +
>
> Unify to just one if condition?

Done.

>>       /* set up the window and fetch the resulting scanner parameters */
>>       status = set_window(pss);
>>       CHECK_STATUS (status, me, "set_window");
>> @@ -1698,6 +1757,12 @@ SANE_Status sane_start (SANE_Handle h)
>>           return status;
>>       }
>>
>> +    if (pss->pdev->model == SCANWIT2720S)
>> +    {
>> +        status = set_frame(pss, pss->frame_no);
>> +        CHECK_STATUS (status, me, "set_frame");
>> +    }
>> +
>>       if (pss->source == SRC_ADF)
>>       {
>>           /* Wait for scanner ready again (e.g. until paper is loaded from an ADF) */
>> diff --git a/backend/snapscan.h b/backend/snapscan.h
>> index 7913aa2..0ab8020 100644
>> --- a/backend/snapscan.h
>> +++ b/backend/snapscan.h
>> @@ -1,9 +1,9 @@
>>   /* sane - Scanner Access Now Easy.
>>
>> -   Copyright (C) 1997, 1998, 1999, 2001  Franck Schnefra, Michel Roelofs,
>> -   Emmanuel Blot, Mikko Tyolajarvi, David Mosberger-Tang, Wolfgang Goeller,
>> -   Petter Reinholdtsen, Gary Plewa, Sebastien Sable, Mikael Magnusson,
>> -   Oliver Schwartz and Kevin Charter
>> +   Copyright (C) 1997, 1998, 1999, 2001, 2002, 2013  Franck Schnefra,
>> +   Michel Roelofs, Emmanuel Blot, Mikko Tyolajarvi, David Mosberger-Tang,
>> +   Wolfgang Goeller, Petter Reinholdtsen, Gary Plewa, Sebastien Sable,
>> +   Mikael Magnusson, Andrew Goodbody, Oliver Schwartz and Kevin Charter
>>
>>      This file is part of the SANE package.
>>
>> @@ -60,6 +60,7 @@
>>
>>   #define DEFAULT_DEVICE "/dev/scanner" /* Check this if config is missing */
>>   #define SNAPSCAN_TYPE      "flatbed scanner"
>> +#define SNAPSCAN_FS_TYPE   "film scanner"
>>   #define TMP_FILE_PREFIX "/var/tmp/snapscan"
>>   #define SNAPSCAN_CONFIG_FILE "snapscan.conf"
>>   #define FIRMWARE_KW "firmware"
>> @@ -107,7 +108,8 @@ typedef enum
>>       PERFECTION2480,     /* Epson Perfection 2480 - 2400 DPI */
>>       PERFECTION3490,     /* Epson Perfection 3490 - 3200 DPI */
>>       STYLUS_CX1500,      /* Epson Stylus CX 1500 - 600 DPI */
>> -    ARCUS1200          /* Agfa Arcus 1200 - 1200 DPI (rebadged Acer?) */
>> +    ARCUS1200,          /* Agfa Arcus 1200 - 1200 DPI (rebadged Acer?) */
>> +    SCANWIT2720S        /* Benq Scanwit 2720S film scanner 2700 DPI */
>>   } SnapScan_Model;
>>
>>   struct SnapScan_Driver_desc {
>> @@ -146,7 +148,8 @@ static struct SnapScan_Driver_desc drivers[] =
>>       {PERFECTION1670, "Perfection 1670"},
>>       {PERFECTION2480, "Perfection 2480"},
>>       {PERFECTION3490, "Perfection 3490"},
>> -    {STYLUS_CX1500,  "Stylus CX 1500"}
>> +    {STYLUS_CX1500,  "Stylus CX 1500"},
>> +    {SCANWIT2720S,   "Benq Scanwit 2720S"}
>
> BenQ ScanWit 2720S

Done.

>>   };
>>
>>   #define known_drivers ((int) (sizeof(drivers)/sizeof(drivers[0])))
>> @@ -200,7 +203,8 @@ static struct SnapScan_Model_desc scanners[] =
>>       {"EPSON Scanner1",      PERFECTION2480}, /* dummy entry to detect scanner */
>>       {"EPSON Scanner2",      PERFECTION3490}, /* dummy entry to detect scanner */
>>       {"EPSON MFP00",            STYLUS_CX1500},
>> -    {"ARCUS 1200",          ARCUS1200}
>> +    {"ARCUS 1200",          ARCUS1200},
>> +    {"FilmScanner____1",    SCANWIT2720S}
>>   };
>>
>>   #define known_scanners ((int) (sizeof(scanners)/sizeof(scanners[0])))
>> @@ -271,6 +275,9 @@ typedef enum
>>       OPT_PREVIEW_MODE,      /* preview mode */
>>       OPT_HIGHQUALITY,       /* scan quality (fast / high) */
>>       OPT_SOURCE,            /* scan source (flatbed / TPO) */
>> +    OPT_FRAME_NO,          /* frame number for film scanner */
>> +    OPT_FOCUS_MODE,        /* manual or auto focus for film scanner */
>> +    OPT_FOCUS_POINT,       /* focus point for film scanner */
>>       OPT_GEOMETRY_GROUP,    /* geometry group */
>>       OPT_TLX,               /* top left x */
>>       OPT_TLY,               /* top left y */
>> @@ -338,6 +345,9 @@ typedef struct snapscan_device
>>   }
>>   SnapScan_Device;
>>
>> +#define MD_AUTO                0
>> +#define MD_MANUAL      1
>> +
>>   #define MAX_SCSI_CMD_LEN 256    /* not that large */
>>   #define DEFAULT_SCANNER_BUF_SZ 1024*63
>>
>> @@ -418,6 +428,10 @@ struct snapscan_scanner
>>       SANE_Bool firmware_loaded;   /* true if firmware was downloaded */
>>       SANE_Word usb_vendor;        /* USB vendor id */
>>       SANE_Word usb_product;       /* USB product id */
>> +    SANE_Byte frame_no;          /* frame number for film scanner */
>> +    SANE_Int focus_mode;         /* focus mode value */
>> +    SANE_String focus_mode_s;    /* focus mode string */
>> +    SANE_Word focus;             /* focus point */
>>   };
>>
>>   #endif
>> --
>> 1.7.10.4
>
> The patch is pretty big. Review would be easier if you factored out the
> bug fixes and maybe add the new options like the frame option in a
> separate patch and then lastly the ScanWit 2720S.

Thanks for working through it. The bug fixes are so small that you would 
gain very little. Splitting the rest up does not really seem right to me.

> Oliver might disagree though.
>
>
> Thanks,
>
> Paul
>
>
> [1] http://git-scm.com/book/en/Customizing-Git-Git-Configuration
>

Andrew



More information about the sane-devel mailing list