2
\$\begingroup\$

Revisions:

png2jpeg is a simple command-line converter of PNG to JPEG.

The source code requires the libpng and libjpeg development libraries to be installed, and can be built with a C99 standard-compliant C compiler.

My intent is to ensure that the source code is readable, understandable, maintainable and has no bugs; in simpler words, that it be of high quality.

Listing of png2jpeg.c:

//
// Copyright (c) 2016 Andrei Bondor
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//

#include <stdbool.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <jpeglib.h>
#include <png.h>

///
/// The version can be thought of as a specially written integer.
/// Thus, for example, the version following "1.0.2.9" is "1.0.3.0".
/// The version shall never be something like "1.0.3210.4".
///
#define PNG2JPEG_VERSION            "1.0.0.1"

///
/// @brief Restricts Var's value to the [Min, Max] interval.
/// @param [in,out] Var             Variable to be clamped.
/// @param [in] Min                 Minimum permitted value for the variable.
/// @param [in] Max                 Maximum permitted value for the variable.
/// @pre `Min < Max`
///
#define CLAMP(Var, Min, Max) if (true) {                                       \
    if ((Var) < (Min)) (Var) = (Min); else                                     \
    if ((Var) > (Max)) (Var) = (Max);                                          \
} else (void)0

///
/// @brief Prints help information.
///
static void print_help(void)
{
    printf("\n===============================================================");
    printf("================\nPNG-TO-JPEG Conversion Utility                 ");
    printf("                 version " PNG2JPEG_VERSION "\n==================");
    printf("=============================================================\n\n");
    printf("The syntax is:\n\n    png2jpeg input_file\n    png2jpeg [option <");
    printf("arg> [...]] input_file\n\nThe following options are supported:\n" );
    printf("\n    LONG NAME       SHORTHAND   ARGUMENT        MEANING\n    --");
    printf("-----------------------------------------------------------------");
    printf("--------\n    --help          -h          (none)          print h");
    printf("elp information\n    --verbose       -v          (none)          ");
    printf("print debug information\n    --grayscale     -g          (none)  ");
    printf("        create monochrome JPEG\n    --progressive   -p          (");
    printf("none)          create progressive JPEG\n    --quality       -q   ");
    printf("       0 to 100        set quality\n    --outfile       -o       ");
    printf("   filename        override output filename\n    --sample        ");
    printf("-s          HxV             set component sampling factors\n    -");
    printf("-background    -b          R,G,B           set background color\n");
    printf("    -------------------------------------------------------------");
    printf("--------------\n\nExamples:\n\n    png2jpeg input.png\n    png2jp");
    printf("eg -p --quality 95 --sample 1x1 -o converted.jpeg original.png\n ");
    printf("   png2jpeg -q 50 --background 128,255,0 icon.png\n\n"            );
}

///
/// @brief Prints a complaint about an option and its argument.
/// @param [in] option              Option to complain about.
/// @param [in] arg                 Argument to complain about.
/// @note If both `option` and `arg` are `NULL` then the complaint is generic.
/// @note If `option` is `NULL` then the complaint is about `arg` being invalid.
/// @note If `arg` is `NULL` then the complaint is about `option` missing its
///  argument.
///
static void print_bad_params(const char *option, const char *arg)
{
    if (option == NULL)
    {
        if (arg == NULL)
            fprintf(stderr, "png2jpeg error: bad option and/or argument\n");
        else
            fprintf(stderr, "png2jpeg error: invalid argument \"%s\"\n", arg);
    }
    else
    if (arg == NULL)
    {
        fprintf(stderr, "png2jpeg error: option \"%s\" has no argument\n",
            option);
    }
    else
    {
        fprintf(stderr, "png2jpeg error: option \"%s\" has invalid argument"
            " \"%s\"\n", option, arg);
    }
}

///
/// @brief Checks if an option was given in the parameters.
/// @param argc                     Number of parameters.
/// @param [in] argv                Parameter strings.
/// @param [in] option              Option string to be searched.
/// @pre `argv != NULL && option != NULL`
/// @returns Whether or not the option was given.
/// @retval true                    The option was found.
/// @retval false                   The option was not found.
///
static bool option_given(int argc, char *argv[], const char *option)
{
    for (int i=0; i < argc; ++i)
        if (strcmp(argv[i], option) == 0)
            return true;

    return false;
}

///
/// @brief Searches the parameters and retrieves an argument string.
/// @param argc                     Number of parameters.
/// @param [in] argv                Parameter strings.
/// @param [in] option              Option string to be searched.
/// @pre `argv != NULL && option != NULL`
/// @returns The argument provided for `option`.
/// @retval NULL                    No argument associated with `option`.
///
static const char * option_arg(int argc, char *argv[], const char *option)
{
    for (int i=0; i < argc; ++i)
        if (strcmp(argv[i], option) == 0)
        {
            // NOTE: according to the C99 standard (Section 5.1.2.2.1/2):
            //  "argv[argc] shall be a null pointer"
            return argv[i + 1];
        }

    return NULL;
}

///
/// @brief Holds the settings for the PNG to JPEG conversion.
/// @remarks "MSF" refers to the MAX_SAMP_FACTOR macro.
///
typedef struct
{
    bool        grayscale;          ///< Grayscale JPEG.
    bool        progressive;        ///< Progressive JPEG.
    int         quality;            ///< Quality, 0 to 100.
    int         samp_h;             ///< Sample factor for H, 1 to MSF.
    int         samp_v;             ///< Sample factor for V, 1 to MSF.
    png_color   bgc;                ///< Background color, for transparency.
} png2jpeg_settings;

///
/// @brief Prints the conversion settings.
/// @param [in] config              Configuration to be printed.
///
static void print_settings(const png2jpeg_settings *config)
{
    printf("\nConversion Settings\n-------------------\n");
    printf("Grayscale:          %s\n", config->grayscale ? "YES" : "NO");
    printf("Progressive:        %s\n", config->progressive ? "YES" : "NO");
    printf("Quality:            %d\n", config->quality);
    printf("Sample factors:     %dx%d\n", config->samp_h, config->samp_v);
    printf("Background color:   (%d,%d,%d)\n\n",
        config->bgc.red, config->bgc.green, config->bgc.blue);
}

///
/// @brief Configures the "Grayscale JPEG" option.
/// @param argc                     Number of parameters, from `main()`.
/// @param [in] argv                Parameter strings, from `main()`.
/// @param [in,out] config          Configuration to be updated.
///
static void config_grayscale(int argc, char *argv[], png2jpeg_settings *config)
{
    if (option_given(argc, argv, "--grayscale") ||
        option_given(argc, argv, "-g"))
    {
        config->grayscale = true;
    }
}

///
/// @brief Configures the "Progressive JPEG" option.
/// @param argc                     Number of parameters, from `main()`.
/// @param [in] argv                Parameter strings, from `main()`.
/// @param [in,out] config          Configuration to be updated.
///
static void config_progressive(int argc, char *argv[],
    png2jpeg_settings *config)
{
    if (option_given(argc, argv, "--progressive") ||
        option_given(argc, argv, "-p"))
    {
        config->progressive = true;
    }
}

///
/// @brief Configures the "Quality" option.
/// @note This function may call `exit()`.
/// @param argc                     Number of parameters, from `main()`.
/// @param [in] argv                Parameter strings, from `main()`.
/// @param [in,out] config          Configuration to be updated.
///
static void config_quality(int argc, char *argv[], png2jpeg_settings *config)
{
    const char *temp_option, *temp_arg;

    if (option_given(argc, argv, "--quality"))
    {
        temp_option = "--quality";
        temp_arg = option_arg(argc, argv, "--quality");
    }
    else
    if (option_given(argc, argv, "-q"))
    {
        temp_option = "-q";
        temp_arg = option_arg(argc, argv, "-q");
    }
    else
        return;

    if (temp_arg == NULL)
    {
        print_bad_params(temp_option, NULL);
        exit(EXIT_FAILURE);
    }

    config->quality = strtol(temp_arg, NULL, 10);
    CLAMP(config->quality, 0, 100);
}

///
/// @brief Configures the "Sample Factors" option.
/// @note This function may call `exit()`.
/// @param argc                     Number of parameters, from `main()`.
/// @param [in] argv                Parameter strings, from `main()`.
/// @param [in,out] config          Configuration to be updated.
///
static void config_sample(int argc, char *argv[], png2jpeg_settings *config)
{
    const char *temp_option, *temp_arg;

    if (option_given(argc, argv, "--sample"))
    {
        temp_option = "--sample";
        temp_arg = option_arg(argc, argv, "--sample");
    }
    else
    if (option_given(argc, argv, "-s"))
    {
        temp_option = "-s";
        temp_arg = option_arg(argc, argv, "-s");
    }
    else
        return;

    if (temp_arg == NULL)
    {
        print_bad_params(temp_option, NULL);
        exit(EXIT_FAILURE);
    }

    char temp_sep; // Separator
    int temp_samp_h, temp_samp_v;

    const int temp_ret = sscanf(temp_arg, "%d%c%d",
        &temp_samp_h, &temp_sep, &temp_samp_v);

    if (temp_ret != 3 || (temp_sep != 'x' && temp_sep != '*'))
    {
        print_bad_params(temp_option, temp_arg);
        exit(EXIT_FAILURE);
    }

    CLAMP(temp_samp_h, 1, MAX_SAMP_FACTOR);
    CLAMP(temp_samp_v, 1, MAX_SAMP_FACTOR);

    config->samp_h = temp_samp_h;
    config->samp_v = temp_samp_v;
}

///
/// @brief Configures the "Background Color" option.
/// @note This function may call `exit()`.
/// @param argc                     Number of parameters, from `main()`.
/// @param [in] argv                Parameter strings, from `main()`.
/// @param [in,out] config          Configuration to be updated.
///
static void config_background(int argc, char *argv[], png2jpeg_settings *config)
{
    const char *temp_option, *temp_arg;

    if (option_given(argc, argv, "--background"))
    {
        temp_option = "--background";
        temp_arg = option_arg(argc, argv, "--background");
    }
    else
    if (option_given(argc, argv, "-b"))
    {
        temp_option = "-b";
        temp_arg = option_arg(argc, argv, "-b");
    }
    else
        return;

    if (temp_arg == NULL)
    {
        print_bad_params(temp_option, NULL);
        exit(EXIT_FAILURE);
    }

    int red, green, blue; // Background Color

    const int temp_ret = sscanf(temp_arg, "%d,%d,%d", &red, &green, &blue);

    if (temp_ret != 3)
    {
        print_bad_params(temp_option, temp_arg);
        exit(EXIT_FAILURE);
    }

    CLAMP(red,      0, 255);
    CLAMP(green,    0, 255);
    CLAMP(blue,     0, 255);

    config->bgc.red     = red;
    config->bgc.green   = green;
    config->bgc.blue    = blue;
}

///
/// @brief Converts PNG image to JPEG image.
/// @note This function may call `exit()`.
/// @param [in] input_fn            Input image filename.
/// @param [in] output_fn           Output image filename.
/// @param [in] config              Conversion settings.
/// @param verbose                  Verbose output.
///
static void png_to_jpeg(const char *input_fn, const char *output_fn,
    const png2jpeg_settings *config, bool verbose)
{
    png_image input_img = {
        .opaque     = NULL,
        .version    = PNG_IMAGE_VERSION
    };

    if (png_image_begin_read_from_file(&input_img, input_fn) == 0)
    {
        fprintf(stderr, "png2jpeg error: reading PNG");

        if (input_img.warning_or_error != 0)
            fprintf(stderr, ", %s\n", input_img.message);
        else
            fprintf(stderr, "\n");

        exit(EXIT_FAILURE);
    }

    if (config->grayscale)
        input_img.format = PNG_FORMAT_GRAY;
    else
        input_img.format = PNG_FORMAT_RGB;

    if (verbose)
        // TODO: cast to size_t and use "%zu" in the future (MinGW 32-bit bug)
        printf("PNG image size in memory: %lu bytes\n",
            (long unsigned int)PNG_IMAGE_SIZE(input_img));

    png_bytep buffer = malloc(PNG_IMAGE_SIZE(input_img));

    if (buffer == NULL)
    {
        // TODO: cast to size_t and use "%zu" in the future (MinGW 32-bit bug)
        fprintf(stderr, "png2jpeg error: out of memory (%lu bytes)\n",
            (long unsigned int)PNG_IMAGE_SIZE(input_img));
        png_image_free(&input_img);
        exit(EXIT_FAILURE);
    }

    if (png_image_finish_read(&input_img, &config->bgc, buffer, 0, NULL) == 0)
    {
        fprintf(stderr, "png2jpeg error: reading PNG");

        if (input_img.warning_or_error != 0)
            fprintf(stderr, ", %s\n", input_img.message);
        else
            fprintf(stderr, "\n");

        free(buffer);
        exit(EXIT_FAILURE);
    }

    FILE *output_img = fopen(output_fn, "wb");

    if (output_img == NULL)
    {
        fprintf(stderr, "png2jpeg error: could not open output file "
            "\"%s\" for writing\n", output_fn);
        free(buffer);
        exit(EXIT_FAILURE);
    }

    struct jpeg_compress_struct     cinfo;
    struct jpeg_error_mgr           jerr;

    cinfo.err = jpeg_std_error(&jerr);
    jpeg_create_compress(&cinfo);
    jpeg_stdio_dest(&cinfo, output_img);
    cinfo.image_width   = input_img.width;
    cinfo.image_height  = input_img.height;

    if (config->grayscale)
    {
        cinfo.input_components  = PNG_IMAGE_SAMPLE_CHANNELS(PNG_FORMAT_GRAY);
        cinfo.in_color_space    = JCS_GRAYSCALE;
    }
    else
    {
        cinfo.input_components  = PNG_IMAGE_SAMPLE_CHANNELS(PNG_FORMAT_RGB);
        cinfo.in_color_space    = JCS_RGB;
    }

    const int   row_stride = cinfo.image_width * cinfo.input_components;
    JSAMPROW    row_pointer[1];

    jpeg_set_defaults(&cinfo);
    jpeg_set_quality(&cinfo, config->quality, false);
    cinfo.optimize_coding = true;

    // TODO: add support for multiple sample factors, as
    //  in `set_sample_factors()` from "rdswitch.c"
    cinfo.comp_info[0].h_samp_factor = config->samp_h;
    cinfo.comp_info[0].v_samp_factor = config->samp_v;

    if (config->progressive)
        jpeg_simple_progression(&cinfo);

    jpeg_start_compress(&cinfo, true);

    while (cinfo.next_scanline < cinfo.image_height)
    {
        row_pointer[0] = &buffer[cinfo.next_scanline * row_stride];
        jpeg_write_scanlines(&cinfo, row_pointer, 1);
    }

    jpeg_finish_compress(&cinfo);
    jpeg_destroy_compress(&cinfo);
    fclose(output_img);
    free(buffer);
}

///
/// @brief Enters the program.
/// @param argc                     Number of parameters.
/// @param [in] argv                Parameter strings.
/// @returns Whether or not the operation was successful.
/// @retval EXIT_SUCCESS            Operation success.
/// @retval EXIT_FAILURE            Operation failure.
///
int main(int argc, char *argv[])
{
    // sets the verbosity of the program
    bool verbose = false;

    // PNG to JPEG conversion settings
    png2jpeg_settings config = {
        .grayscale      = false,
        .progressive    = false,
        .quality        = 75,
        .samp_h         = 2,
        .samp_v         = 2,
        .bgc            = {.red = 0, .green = 0, .blue = 0}
    };

    if (argc <= 1 ||
        option_given(argc, argv, "--help") ||
        option_given(argc, argv, "-h"))
    {
        print_help();
        return EXIT_SUCCESS;
    }

    if (option_given(argc, argv, "--verbose") ||
        option_given(argc, argv, "-v"))
    {
        verbose = true;
    }

    config_grayscale    (argc, argv, &config);
    config_progressive  (argc, argv, &config);
    config_quality      (argc, argv, &config);
    config_sample       (argc, argv, &config);
    config_background   (argc, argv, &config);

    if (verbose)
        print_settings(&config);

    // Input and Output Image Filenames
    // NOTE: (argc - 1) is safe as (argc <= 1) was checked before getting here
    const char * const input_fn = argv[argc - 1];
    const char *output_fn = NULL;

    if (option_given(argc, argv, "--outfile"))
        output_fn = option_arg(argc, argv, "--outfile");
    else
    if (option_given(argc, argv, "-o"))
        output_fn = option_arg(argc, argv, "-o");

    // Output Image Automatic Extension (period included)
    const char * const output_ext = ".jpg";

    // Output Image Automatic Filename
    // FIXME: potential stack overflow vulnerability!
    char output_autofn[strlen(input_fn) + strlen(output_ext) + 1];

    // check if user overrode the automatic filename; if not, use it
    if (output_fn == NULL)
    {
        strcpy(output_autofn, input_fn);
        strcat(output_autofn, output_ext);
        output_fn = output_autofn;
    }

    if (verbose)
    {
        printf("Input image filename: \"%s\"\n", input_fn);
        printf("Output image filename: \"%s\"\n", output_fn);
    }

    png_to_jpeg(input_fn, output_fn, &config, verbose);
    return EXIT_SUCCESS;
}
\$\endgroup\$
  • \$\begingroup\$ For those who are repelled by print_help(), this function is generated and the source code for the generator is on the source forge site at sourceforge.net/p/png2jpeg/code/HEAD/tree in the nodlist directory. I'm going to wait 2 days to let others review this code first and only answer if no one else does (I may have done enough damage already and I have bad eyes.). \$\endgroup\$ – pacmaninbw Apr 17 '16 at 17:34
  • 1
    \$\begingroup\$ WRT print_help(), the packing of the text is the smallest problem. You're using arbitrary text as a format string to printf, and your escaping is insufficient (only taking care of quotes and newline characters). Better to make only a single function call (the preprocessor combines adjacent whitespace literals on different lines just as happily as it combines deal with your PNG2JPEG_VERSION) and use a string-printing function (puts) rather than a format-string-interpreting function (printf). \$\endgroup\$ – Ben Voigt Apr 17 '16 at 20:35
3
\$\begingroup\$

Keep in mind these are suggestions, your code already works.

Generated Code:

Never mix generated code with hand written code. Generated code should be in a file that compiles by itself. It is too easy to make mistakes when puting generated code into a hand coded file.

The makefile should be modified to generate print_help.c and then build the print_help object from print_help.c.

Alter your generator so print_help.c includes any header files it needs to and so it generates print_help.h which has the declaration for the print_help() function. Modify png2jpeg.c so that it includes print_help.h.

Makefile:

The GNUmakefile is not shown here and I shouldn't comment on it but this will help you in the future.

Automate as much of the build process as possible to remove human error. A build that includes the generation of the generated files reduces possible build errors. You already have examples of how to do this in the libraries you are using (libpng and libjpeg). Both libraries provide an executable configure file which builds the makefile. The build process for both of them is ./configure, make, make install.

As your programs grow in complexity you may decide to have multiple source files, main.c, command_line.c, command_line.h, algorithm.c, algorithm.h, version_number.h, print_help.h and print_help.c. The makefile should be changed to build multiple objects from multiple source files, and should have a separate linking step.

You can also add testing steps to you make file to automatically test your product. This can provide regression testing to see if any new changes broke previously working code.

Main:

@Bizkit has pretty much covered main(), and command line processing this time.

The current design forces the execution of functions that may not need to execute. By using the for loop in @Bizkit's setup_config() function the program only executes necessary functions.

What is very good about @Bizkit's answer is that it only loops through argv once rather than multiple times. The current implementation loops through argv at least once for all options and twice for some options. His answer is a fairly common way of addressing command line arguments.

The other good thing about Bizkit's answer is that all the variables for the command line options are stored in the png2jpeg_settings structure.

Another example of command line parsing is check_flags in this question.

Here are corrections to Bizkit's answer which is generally correct:

void setup_outfile(char *next_arg, png2jpeg_settings *config)
{
    char *outfile = malloc(strlen(next_arg) + 1);
    strcpy(outfile, next_arg);
    config->outfile = outfile;
}

void use_default_output_file_when_necessary(png2jpeg_settings *config)
{
    const char output_ext[] = ".jpg";

    // Output Image Automatic Filename
    char *outfile = malloc(strlen(config->infile) + sizeof(output_ext) + 1);

    // check if user overrode the automatic filename; if not, use it
    if (config->outfile == NULL)
    {
         strcpy(outfile, config->infile);
         strcat(outfile, output_ext);
         config->outfile = outfile;
    }
}

static void setup_config(int argc, char **argv, png2jpeg_settings *config)
{
    for (int i = 1; i < argc; ++i) {
        if (alt_strcmp(argv[i], "--quality", "-q")) {
            process_next_arg(argv[i + 1], config, setup_quality);
        } else if (alt_strcmp(argv[i], "--progressive", "-p")) {
            config->progressive = true;
        } else if (alt_strcmp(argv[i], "--grayscale", "-g")) {
            config->grayscale = true;
        } else if (alt_strcmp(argv[i], "--background", "-b")) {
            process_next_arg(argv[i + 1], config, setup_bg);
        } else if (alt_strcmp(argv[i], "--sample", "-s")) {
            process_next_arg(argv[i + 1], config, setup_sample);
        } else if (alt_strcmp(argv[i], "--outfile", "-o")) {
            process_next_arg(argv[i + 1], config, setup_outfile);
        } else if (alt_strcmp(argv[i], "--verbose", "-v")) {
            config->verbose = true;
        } else if (alt_strcmp(argv[i], "--help", "-h")) {
            print_help();
            exit(EXIT_SUCCESS);
        } else {
            // handle error
        }
    }

    config->infile = argv[argc - 1];

    use_default_output_file_when_necessary(config);
}

I have modified Bizkit's main slightly by adding a free and returning status from png_to_jpeg which now takes the struct rather than a pointer to the struct because there is no need to modify config in png_to_jpeg().

int main(int argc, char **argv)
{
    int exit_status = EXIT_SUCCESS;
    if (argc <= 1) {
        print_help();
        return exit_status;
    }

    // PNG to JPEG conversion settings
    png2jpeg_settings config = {
        .bgc            = {.red = 0, .green = 0, .blue = 0},
        .outfile        = NULL,
        .infile         = NULL,
        .quality        = 75,
        .samp_h         = 2,
        .samp_v         = 2,
        .grayscale      = false,
        .progressive    = false,
        .verbose        = false
    };

    setup_config(argc, argv, &config);

    if (config.verbose) {
        printf("Input image filename: \"%s\"\n", config.infile);
        printf("Output image filename: \"%s\"\n", config.outfile);
    }

    // we now have all the info we need in the config structure. So we change
    // the signature to only include our config structure
    exit_status = png_to_jpeg(config);

    free(config.outfile);
    return exit_status;
}

Function Size in Lines of Code:

Functions should be small enough to read easily. They should complete one particular action (atomic). This so that they can be more easily written and debugged. The more complex a function is, the harder it is to understand, write, debug and maintain. If possible they should be reusable. Functions that are longer than one screen are generally too complex to understand easily and lead to maintenance problems (where do I make the edit?), and they can't be reused.

Variables that will be modified by the function should be passed in by reference or by name, variables that will not be modified should be passed in by value. Global variables are discouraged because they lead to bugs and it's very hard to trace their value through the program.

main

The main() function is still long enough to require 2 screens to see all of it. There should be a second grayscale function for the override. The call to the second grayscale function could be part of png to jpeg conversion, it logically belongs there.

png_to_jpeg

From the keyword static to the closing right brace, the function png_to_jpeg() is 110 lines, including the comments it requires 2 to 3 screens to see the entire function. It is also a fairly complex function. I see the possibility of breaking png_to_jpeg into at least 3 functions. This uses Bizkit's definition of png2jpeg_settings.

/*
 * Assume this is new png2jpeg_settings structure. Note that I've rearranged 
 * it in descending order by the sizes of the member elements. This improves
 * the space locality of the structure by reducing padding and thus the 
 * overall size of the structure
 */
typedef struct {
    png_color bgc;
    char *outfile, *infile;
    int quality, samp_h, samp_v;
    bool verbose, grayscale, progressive;
} png2jpeg_settings;

static void finish_grayscale_configuration(png2jpeg_settings *config,
    png_image *input_img)
{
    if (config->grayscale)
    {
        const int gray =
            (config->bgc.red + config->bgc.green + config->bgc.blue) / 3;

        config->bgc.red      = gray;
        config->bgc.green    = gray;
        config->bgc.blue     = gray;

        input_img->format = PNG_FORMAT_GRAY;
    }
    else
    {
        input_img->format = PNG_FORMAT_RGB;
    }
}

static int initialize_input_img(png2jpeg_settings config,
    png_image *input_img)
{
    int exit_status = EXIT_SUCCESS;

    input_img->opaque = NULL;
    input_img->version = PNG_IMAGE_VERSION;

    if (png_image_begin_read_from_file(input_img, config.infile) == 0)
    {
        fprintf(stderr, "png2jpeg error: reading PNG");

        if (input_img->warning_or_error != 0)
            fprintf(stderr, ", %s\n", input_img->message);
        else
            fprintf(stderr, "\n");

        exit_status = EXIT_FAILURE;
    }

    return exit_status;
}

static int process_original_png_file(png2jpeg_settings config,
    png_image *input_img, png_bytep buffer)
{
    int exit_status = EXIT_SUCCESS;

    if (png_image_finish_read(input_img, &config.bgc, buffer, 0, NULL) == 0)
    {
        fprintf(stderr, "png2jpeg error: reading PNG");

        if (input_img->warning_or_error != 0)
            fprintf(stderr, ", %s\n", input_img->message);
        else
            fprintf(stderr, "\n");

        exit_status = EXIT_FAILURE;
    }

    return exit_status;
}

static int generate_jpeg_output_file(png2jpeg_settings config,
     png_image input_img, png_bytep buffer)
{
    struct jpeg_compress_struct     cinfo;
    struct jpeg_error_mgr           jerr;
    int exit_status = EXIT_SUCCESS;
    FILE *output_img = fopen(config.outfile, "wb");

    if (output_img == NULL)
    {
        fprintf(stderr, "png2jpeg error: could not open output file "
            "\"%s\" for writing\n", config.outfile);
        return EXIT_FAILURE;
    }

    cinfo.err = jpeg_std_error(&jerr);
    jpeg_create_compress(&cinfo);
    jpeg_stdio_dest(&cinfo, output_img);
    cinfo.image_width   = input_img.width;
    cinfo.image_height  = input_img.height;

    if (config.grayscale)
    {
        cinfo.input_components  = PNG_IMAGE_SAMPLE_CHANNELS(PNG_FORMAT_GRAY);
        cinfo.in_color_space    = JCS_GRAYSCALE;
    }
    else
    {
        cinfo.input_components  = PNG_IMAGE_SAMPLE_CHANNELS(PNG_FORMAT_RGB);
        cinfo.in_color_space    = JCS_RGB;
    }

    const int   row_stride = cinfo.image_width * cinfo.input_components;
    JSAMPROW    row_pointer[1];

    jpeg_set_defaults(&cinfo);
    jpeg_set_quality(&cinfo, config.quality, false);
    cinfo.optimize_coding = true;

    // TODO: add support for multiple sample factors, as
    //  in `set_sample_factors()` from "rdswitch.c"
    cinfo.comp_info[0].h_samp_factor = config.samp_h;
    cinfo.comp_info[0].v_samp_factor = config.samp_v;

    if (config.progressive)
        jpeg_simple_progression(&cinfo);

    jpeg_start_compress(&cinfo, true);

    while (cinfo.next_scanline < cinfo.image_height)
    {
        row_pointer[0] = &buffer[cinfo.next_scanline * row_stride];
        jpeg_write_scanlines(&cinfo, row_pointer, 1);
    }

    jpeg_finish_compress(&cinfo);
    jpeg_destroy_compress(&cinfo);
    fclose(output_img);

    return exit_status;
}

static int png_to_jpeg(png2jpeg_settings config)
{
    int exit_status = EXIT_SUCCESS;
    png_image input_img;
    png_bytep buffer;

    if ((exit_status = initialize_input_img(config, &input_img)) !=
        EXIT_SUCCESS)
        return exit_status;

    if (config.verbose)
        // TODO: cast to size_t and use "%zu" in the future (MinGW 32-bit bug)
        printf("PNG image size in memory: %lu bytes\n",
            (long unsigned int)PNG_IMAGE_SIZE(input_img));

    buffer = malloc(PNG_IMAGE_SIZE(input_img));
    if (buffer == NULL)
    {
        exit_status = EXIT_FAILURE;
        // TODO: cast to size_t and use "%zu" in the future (MinGW 32-bit bug)
        fprintf(stderr, "png2jpeg error: out of memory (%lu bytes)\n",
            (long unsigned int)PNG_IMAGE_SIZE(input_img));
        png_image_free(&input_img);
        return exit_status;
    }

    exit_status = process_original_png_file(config, &input_img, buffer);

    if (exit_status == EXIT_SUCCESS)
    {
        finish_grayscale_configuration(&config, &input_img);
        exit_status = generate_jpeg_output_file(config, input_img, buffer);
    }

    free(buffer);

    return exit_status;
}

The exit(EXIT_FAILURE) statement have been changed to returns. It png2jpeg you know that it would be ok to exit, but in some programs you won't be able to know if it is ok to exit. It's better to get into good habits that allow main() to handle the exit() whenever possible. There may be other FILE pointers that need to close, there may be other malloced blocks that need to be freed. The portion of the program you are working on may be part of the operating system and can't exit.

You also might want to do some research on signal handlers and setjmp and longjmp.

Current Commandline Functions:

The current commandline functions aren't wrong but they can be improved, please note that @Bizkit addressed some of the items I discuss here in a different manner, and overall is probably a better solution.

static void config_quality(int argc, char *argv[], png2jpeg_settings *config)
{
    const char *temp_option, *temp_arg;

    if (option_given(argc, argv, "--quality"))
    {
        temp_option = "--quality";
        temp_arg = option_arg(argc, argv, "--quality");
    }
    else
    if (option_given(argc, argv, "-q"))
    {
        temp_option = "-q";
        temp_arg = option_arg(argc, argv, "-q");
    }
    else
        return;

    if (temp_arg == NULL)
    {
        print_bad_params(temp_option, NULL);
        exit(EXIT_FAILURE);
    }

    config->quality = strtol(temp_arg, NULL, 10);
    CLAMP(config->quality, 0, 100);
}

The possible issues with the code above are : 1) It loops through argv twice, which time consuming and wastefull. 2) Getting the argument is common to both branches of the if statement so it should be removed from the if statement. The possible problem is in maintenance, one branch of the if may be modified, and the the modifier may miss the other edit.

In the existing code option, option_given() returns a bool, a better implementation might be to use get_option_index() which returns the index in argv of the option

static bool get_option_index(int argc, char *argv[], const char *option)
{
    for (int i = 1; i < argc; ++i)
        if (strcmp(argv[i], option) == 0)
            return i;

    return 0;
}

static void config_quality(int argc, char *argv[], png2jpeg_settings *config)
{
    const char *temp_option, *temp_arg; 
    int option_index;

    option_index = get_option_index(argc, argv, "--quality");
    if (option_index < 1)
    {
        option_index = get_option_index(argc, argv, "-q");
        if (option_index < 1)
        {
            return;     // option not found.
        }
    }

    temp_option = argv[option_index];
    option_index++;
    if (option_index >= argc) // prevents indexing past the end of argv.
    {
        print_bad_params(temp_option, NULL);
        exit(EXIT_FAILURE);
    }
    temp_arg = argv[option_index];

    config->quality = strtol(temp_arg, NULL, 10);
    CLAMP(config->quality, 0, 100);
}

Comments:

Unless you are using doxygen to document your software, you really only need the @brief and @note comments. Otherwise your comment blocks will become a maintenance problem. Any change to a function will require changes to the comments as well. The best comments I see are the TODO ones, and the NOTE about grayscale.

\$\endgroup\$
  • 1
    \$\begingroup\$ Really excellent answer! Thanks for pointing out the issues in my answer. One thing in your answer though where you mention "png_to_jpeg which now takes the struct rather than a pointer to the struct because there is no need to modify config in png_to_jpeg()". Why pass in the structure by value and incur a copy when you can pass in by pointer to const? I'm curious to know your reason. \$\endgroup\$ – Bizkit Apr 20 '16 at 13:57
  • 1
    \$\begingroup\$ @Bizkit that particular copy is not a performance issue, and the compiler itself will probably optimize it. Certain parts of png_to_jpeg() will be processor intensive such as the actual algorithm to perform the conversion. Pass by value won't affect that CPU intensive portion. There is no reason to globally modify the contents of config beyond this point. \$\endgroup\$ – pacmaninbw Apr 20 '16 at 14:08
  • \$\begingroup\$ I am considering to split up the png_to_jpeg() function into several parts, however I am concerned that the result will be three or more functions completely meaningless on their own, which will not help understandability. As for the command-line options, I've done something about that in Revision 3 which, if bug-free, looks quite elegant IMO. \$\endgroup\$ – user102756 Apr 20 '16 at 16:43
3
\$\begingroup\$

As it stands, parsing the input is taking you \$\mathcal{O}(N \times M)\$ where N is the number of configuration options you're looking to fill and M is the number of options given at the command line. You can actually reduce this to \$\mathcal{O} (M)\$ if you do the below:

/* Returns whether the string @param base is equal to __either__ one of
   string @param s1 or string @param s2
 */
bool alt_strcmp(const char *base, const char *s1, const char *s2)
{
    return strcmp(base, s1) == 0 || strcmp(base, s2) == 0;
}

/* Callback function that sets up a particular configuration option
   based on the next command line argument */
typedef void (*setupFxn)(char *temp_arg, png2jpeg_settings *config);

/* Checks to see if the next argument supplied was valid. If it is, 
   it calls the provided callback function using next_arg and config. 
   If it isn't, the function calls exit() 
 */
void process_next_arg(char *next_arg, png2jpeg_settings *config, setupFxn callback) {
    if (!next_arg) {
        // handle error
    } else {
        callback(next_arg, config);
    }
}

void setup_bg(char *temp_arg, png2jpeg_settings *config)
{
    int red, green, blue; // Background Color

    const int temp_ret = sscanf(temp_arg, "%d,%d,%d", &red, &green, &blue);

    if (temp_ret != 3)
    {
        print_bad_params(temp_option, temp_arg);
        exit(EXIT_FAILURE);
    }

    CLAMP(red,      0, 255);
    CLAMP(green,    0, 255);
    CLAMP(blue,     0, 255);

    config->bgc.red     = red;
    config->bgc.green   = green;
    config->bgc.blue    = blue;
}

void setup_sample(char *temp_arg, png2jpeg_settings *config)
{
    char temp_sep; // Separator
    int temp_samp_h, temp_samp_v;

    const int temp_ret = sscanf(temp_arg, "%d%c%d",
    &temp_samp_h, &temp_sep, &temp_samp_v);

    if (temp_ret != 3 || (temp_sep != 'x' && temp_sep != '*'))
    {
        print_bad_params(temp_option, temp_arg);
        exit(EXIT_FAILURE);
    }

    CLAMP(temp_samp_h, 1, MAX_SAMP_FACTOR);
    CLAMP(temp_samp_v, 1, MAX_SAMP_FACTOR);

    config->samp_h = temp_samp_h;
    config->samp_v = temp_samp_v;
}

void setup_quality(char *next_arg, png2jpeg_settings *config)
{
     config->quality = strtol(next_arg, NULL, 10);
     CLAMP(config->quality, 0, 100);        
}

void setup_outfile(char *next_arg, png2jpeg_settings *config)
{
    // Output Image Automatic Extension (period included)
    const char output_ext[] = ".jpg";

    // Output Image Automatic Filename
    // FIXME: potential stack overflow vulnerability!
    char outfile[strlen(config->infile) + sizeof(output_ext)];

    // check if user overrode the automatic filename; if not, use it
    if (outfile == NULL)
    {
        strcpy(outfile, config->infile);
        strcat(outfile, output_ext);
        config->outfile = outfile;
    }
}

void setup_config(int argc, char **argv, png2jpeg_settings *config)
{
    for (int i = 1; i < argc; ++i) {
        if (alt_strcmp(argv[i], "--quality", "-q")) {
            process_next_arg(argv[i + 1], config, setup_quality);
        } else if (alt_strcmp(argv[i], "--progressive", "-p")) {
            config->progressive = true;
        } else if (alt_strcmp(argv[i], "--grayscale", "-g")) {
            config->grayscale = true;
        } else if (alt_strcmp(argv[i], "--background", "-b")) {
            process_next_arg(argv[i + 1], config, setup_bg);
        } else if (alt_strcmp(argv[i], "--sample", "-s")) {
            process_next_arg(argv[i + 1], config, setup_sample);
        } else if (alt_strcmp(argv[i], "--outfile", "-o")) {
            process_next_arg(argv[i + 1], config, setup_outfile);
        } else if (alt_strcmp(argv[i], "--verbose", "-v")) {
            config->verbose = true;
        } else if (alt_strcmp(argv[i], "--help", "-h")) {
            print_help();
            exit();
        } else {
            // handle error
        }
    }
    config->infile = argv[argc - 1];
}

All option_arg does is return the next argument if the option argument is found. However, we know that the argument was found since we looked for it in the above code. So it's OK to just supply the next argument.

This code would just require you to add the name of the output file, input file, and verbosity to your configuration structure (where they should be anyway since they are configuration parameters).

You can then replace all of main with the following:

/* Assume this is new png2jpeg_settings structure. Note that I've rearranged 
   it in descending order by the sizes of the member elements. This improves
   the space locality of the structure by reducing padding and thus the 
   overall size of the structure
 */
typedef struct {
    png_color bgc;
    char *outfile, *infile;
    int quality, samp_h, samp_v;
    bool verbose, grayscale, progressive;
} png2jpeg_settings;

int main(int argc, char **argv)
{
    if (argc <= 1) {
        print_help();
        return EXIT_SUCCESS;    
    }

    // PNG to JPEG conversion settings
    png2jpeg_settings config = {
        .bgc            = {.red = 0, .green = 0, .blue = 0},
        .outfile        = NULL,
        .infile         = NULL,
        .quality        = 75,
        .samp_h         = 2,
        .samp_v         = 2,
        .grayscale      = false,
        .progressive    = false,
        .verbose        = false
    };

    setup_config(argc, argv, &config);

    if (config.verbose) {
        printf("Input image filename: \"%s\"\n", config.infile);
        printf("Output image filename: \"%s\"\n", config.outfile);
    }

    // we now have all the info we need in the config structure. So we change
    // the signature to only include our config structure
    png_to_jpeg(&config);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ In main() config->???? should be config.???? It might be better to start the loop at argv[1] stackoverflow.com/questions/2050961/… . It doesn't handle the special case of no arguments properly, which is print_help(). \$\endgroup\$ – pacmaninbw Apr 19 '16 at 11:10
  • \$\begingroup\$ You're correct. I'll edit the answer. \$\endgroup\$ – Bizkit Apr 19 '16 at 13:55
  • \$\begingroup\$ I am uncertain if this should be implemented. First, the idea to optimize the parsing of user parameters seems over-the-top for this program. Second, I think it makes the source code harder to understand by adding more moving parts, which also increases the likelihood of bugs being introduced: in your code, when I wrote this reply, config->outfile may end up pointing at a stack-allocated variable and I can't think of any elegant way to fix that. But +1 for the thought-provoking post. \$\endgroup\$ – user102756 Apr 19 '16 at 23:01
  • \$\begingroup\$ setup_outfile(char *next_arg, png2jpeg_settings *config) is wrong because the code ignores next_arg which is the user specified output file name. The code to provide a default output file probably should not be in that function, or it should use strdup to malloc the file name stackoverflow.com/questions/252782/strdup-what-does-it-do-in-c which would allocate space from the heap. Overall the approach @Bizkit proposes is a fairly common way to process command line arguments. \$\endgroup\$ – pacmaninbw Apr 20 '16 at 1:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.