png2jpeg, a utility for converting PNG to JPEG (rev. 1/3)

Question

Revisions:

• Revision 1 (you are here)
• Revision 2
• Revision 3

I've been working on a simple command-line png2jpeg utility. Its purpose is to convert images from PNG format 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.

Before I change the SourceForge project status to "production" and upload binary files, I would like the current "beta" to be tested and reviewed. I am most concerned about bugs, coding mistakes and memory leaks.

Official page is:
https://sourceforge.net/p/png2jpeg/code/HEAD/tree/

Listing of first revision png2jpeg.c source file:
https://sourceforge.net/p/png2jpeg/code/1/tree//png2jpeg.c

Content 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.0"

///
/// @brief Restricts the variable's value in 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("================================================================" );
    printf("\n\nThe syntax is:\n\n    png2jpeg input.png\n    png2jpeg [optio");
    printf("n <arg> [...]] input.png\n\nThe following options are supported:" );
    printf("\n\n    LONG NAME       SHORTHAND   ARGUMENT        MEANING\n    ");
    printf("-----------------------------------------------------------------");
    printf("-----------\n    --help          -h          (none)          prin");
    printf("t help 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 colo");
    printf("r\n    ----------------------------------------------------------");
    printf("------------------\n\nExamples:\n\n    png2jpeg input.png\n    pn");
    printf("g2jpeg -p --quality 95 --sample 1x1 -o converted.jpeg input.png"  );
    printf("\n    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 (at 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);
}

/// Determines the verbosity of the program.
static bool verbose = false;

///
/// @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[])
{
    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;
    }

    if (option_given(argc, argv, "--grayscale") ||
        option_given(argc, argv, "-g"))
    {
        config.grayscale = true;
    }

    if (option_given(argc, argv, "--progressive") ||
        option_given(argc, argv, "-p"))
    {
        config.progressive = true;
    }

    if (option_given(argc, argv, "--quality"))
    {
        const char *temp_arg = option_arg(argc, argv, "--quality");

        if (temp_arg == NULL)
        {
            print_bad_params("--quality", NULL);
            return EXIT_FAILURE;
        }

        config.quality = strtol(temp_arg, NULL, 10);
        CLAMP(config.quality, 0, 100);
    }
    else
    if (option_given(argc, argv, "-q"))
    {
        const char *temp_arg = option_arg(argc, argv, "-q");

        if (temp_arg == NULL)
        {
            print_bad_params("-q", NULL);
            return EXIT_FAILURE;
        }

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

    if (option_given(argc, argv, "--sample"))
    {
        const char *temp_arg = option_arg(argc, argv, "--sample");

        if (temp_arg == NULL)
        {
            print_bad_params("--sample", NULL);
            return EXIT_FAILURE;
        }

        char temp_sep; // Separator

        const int temp_ret = sscanf(temp_arg, "%d%c%d",
            &config.samp_h, &temp_sep, &config.samp_v);

        if (temp_ret != 3 || (temp_sep != 'x' && temp_sep != '*'))
        {
            print_bad_params("--sample", temp_arg);
            return EXIT_FAILURE;
        }

        CLAMP(config.samp_h, 1, MAX_SAMP_FACTOR);
        CLAMP(config.samp_v, 1, MAX_SAMP_FACTOR);
    }
    else
    if (option_given(argc, argv, "-s"))
    {
        const char *temp_arg = option_arg(argc, argv, "-s");

        if (temp_arg == NULL)
        {
            print_bad_params("-s", NULL);
            return EXIT_FAILURE;
        }

        char temp_sep; // Separator

        const int temp_ret = sscanf(temp_arg, "%d%c%d",
            &config.samp_h, &temp_sep, &config.samp_v);

        if (temp_ret != 3 || (temp_sep != 'x' && temp_sep != '*'))
        {
            print_bad_params("-s", temp_arg);
            return EXIT_FAILURE;
        }

        CLAMP(config.samp_h, 1, MAX_SAMP_FACTOR);
        CLAMP(config.samp_v, 1, MAX_SAMP_FACTOR);
    }

    if (option_given(argc, argv, "--background"))
    {
        const char *temp_arg = option_arg(argc, argv, "--background");

        if (temp_arg == NULL)
        {
            print_bad_params("--background", NULL);
            return EXIT_FAILURE;
        }

        int red, green, blue;

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

        if (temp_ret != 3)
        {
            print_bad_params("--background", temp_arg);
            return 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;
    }
    else
    if (option_given(argc, argv, "-b"))
    {
        const char *temp_arg = option_arg(argc, argv, "-b");

        if (temp_arg == NULL)
        {
            print_bad_params("-b", NULL);
            return EXIT_FAILURE;
        }

        int red, green, blue;

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

        if (temp_ret != 3)
        {
            print_bad_params("-b", temp_arg);
            return 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;
    }

    if (verbose)
        print_settings(&config);

    png_image input_img = {
        .opaque     = NULL,
        .version    = PNG_IMAGE_VERSION
    };

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

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

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

    // Output Image Automatic Filename
    char output_img_autofn[strlen(input_img_fn) + strlen(output_img_ext) + 1];

    // check if user overrided the automatic filename; if not, use it
    if (output_img_fn == NULL)
    {
        output_img_autofn[0] = '\0';
        strcpy(output_img_autofn, input_img_fn);
        strcat(output_img_autofn, output_img_ext);
        output_img_fn = output_img_autofn;
    }

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

    if (png_image_begin_read_from_file(&input_img, input_img_fn) != 0)
    {
        if (config.grayscale)
            input_img.format = PNG_FORMAT_GRAY;
        else
            input_img.format = PNG_FORMAT_RGB;

        if (verbose)
            printf("PNG image size in memory: %u bytes.\n",
                PNG_IMAGE_SIZE(input_img));

        png_bytep buffer = malloc(PNG_IMAGE_SIZE(input_img));

        if (buffer == NULL)
        {
            fprintf(stderr, "png2jpeg error: out of memory (%u bytes).\n",
                PNG_IMAGE_SIZE(input_img));
            png_image_free(&input_img);
            return EXIT_FAILURE;
        }

        if (png_image_finish_read(&input_img, &config.bgc, buffer, 0, NULL)
            != 0)
        {
            FILE *output_img = fopen(output_img_fn, "wb");

            if (output_img != NULL)
            {
                struct jpeg_compress_struct cinfo;
                struct jpeg_error_mgr jerr;
                JSAMPROW row_pointer[1];
                int row_stride;

                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;
                }

                row_stride = cinfo.image_width * cinfo.input_components;
                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);
            }
            else
            {
                fprintf(stderr, "png2jpeg error: could not open output file "
                    "\"%s\" for writing.\n", output_img_fn);
                free(buffer);
                return EXIT_FAILURE;
            }
        }
        else
        {
            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);
            return EXIT_FAILURE;
        }

        free(buffer);
    }
    else
    {
        fprintf(stderr, "png2jpeg error: reading PNG");

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

        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

Answer 1

Welcome to CodeReview First, this is a great first effort on CodeReview.

Indicate in both the source code and the readme.txt file the actual version number of libjpeg and libpng that you are using. It will help users to have the correct environment to build the program.

Memory Leaks: There does not appear to be a memory leak, but in a single execution program such as this that is less of a problem. Where there might be a memory related problem is that there are multiple free statements for one malloc(). To get around this possible problem the code could be re-factored in this manner:

int FUNCTION_NAME(args)
{
    int exitStatus = EXIT_SUCCESS;
    ptrType *mallocedPtr = NULL;

    mallocedPtr = malloc(MALLOC_SIZE);
    if (mallocedPtr) {
        exitStatus = function(mallocedPtr, ...);
        free(mallocedPtr);
        mallocedPtr = NULL;
    }
    else
    {
        exitStatus = EXIT_FAILURE;
    }

    return exitStatus;
}

The free() function only deallocates the memory, it does not clear the pointer value. When I use malloc and free I define a FREE macro:

#define FREE(allocatedMemoryPtr)   \
    if (allocatedMemoryPtr) {      \
        free(allocatedMemoryPtr);  \
        allocatedMemoryPtr = NULL; \
    }

I do this to prevent freeing memory multiple times, because this can cause the program to terminate.

Function Length: The function main() is over 300 lines long. This makes it harder to read, modify and debug.

Generally a function shouldn't be any longer than what can be displayed in a single editing screen without scrolling.

There is a fair amount of code handling the options that repeats. Code that repeats should be in functions, so that it can be written once and used multiple times. It will also only need to be debugged once and maintenance is simplilifed because if the code needs to be changed it only needs to be changed in one place.

The main() should be broken up in to several functions.

Magic Numbers

There are raw numeric constants in the code:

    png2jpeg_settings config = {
        .grayscale      = false,
        .progressive    = false,
        .quality        = 75,
        .samp_h         = 2,
        .samp_v         = 2,
        .bgc            = {.red = 0, .green = 0, .blue = 0}
    };

        CLAMP(config.quality, 0, 100);

        if (temp_ret != 3 || (temp_sep != 'x' && temp_sep != '*'))

        output_img_autofn[0] = '\0';

The code will be much more readable and easier to maintain if the numbers are replaced with sybolic constants. When using g++ define constants using

static const int DEFAULT_SAMPLE_FACTOR = 2;

When using gcc

#define DEFAULT_SAMPLE_FACTOR 2

Using either gcc or g++

    png2jpeg_settings config = {
        .grayscale      = false,
        .progressive    = false,
        .quality        = 75,
        .samp_h         = DEFAULT_SAMPLE_FACTOR,
        .samp_v         = DEFAULT_SAMPLE_FACTOR,
        .bgc            = {.red = 0, .green = 0, .blue = 0}
    };

In addition to being easier to read, to modify the values you only need to change the code in one place rather than multiple places.

Function Names: Your function names are very readable, however, generally underscores ('_') are no longer used in function names, camelCase is used instead, it is still readable and slightly reduces the length of the function name.

printBadParameters()
optionGiven()
optionArg()
printSettings()

print_help():

This function is very hard to read, and even harder to modify. It should be possible to have a single printf for each option.

Global Variables: While the variable verbose is confined to the file by the use of static, it is still a global variable within the file. It would be best if verbose was declared within main() and passed to other functions. It might be good if verbose was added as a member of the png2jpeg_settings struct, so that all of the program options are in one place.

CLAMP

The macro CLAMP might be better implemented as a function

static int clamp(int clampedVariable, int lowerBound, int upperBound)
{
    if (clampedVariable > upperBound) {
        clampedVariable = upperBound;
    }
    if (clampedVariable > lowerBound) {
        clampedVariable = lowerBound;
    }

    return clampedVariable;
}

CLAMP could also be defined using a ternary operator:

#define CLAMP(Var, Min, Max) Var = ((Var) < (Min))? (Min): ((Var) > (Max))? (Max) : (Var);

A discussions of when to use a macro can be found here and here.

Inconsistent Coding Style:

In some portions of main() you use

if (functionName(args)) {}

but in other portions of main() you use

if (functionName(args) != 0) {}

These 2 uses are equivalent. Stick with one, rather than using both.

Answer 2

To expand on pacmaninbw's answer, I think it would be great to have a single function called parse_input or something similar that fills a predefined png2jpeg_settings structure (like the one you have). That way, I only need to parse the input once during the entire program as well as have a very simple main function:

void parse_input_for_config(int argc, char **argv, png2jpeg_settings *config)
{
    // I think most of this function could be implemented using the strtok
    // standard library function. In this function, you fill the config
    // structure with all of the necessary information from user input. No
    // checking of info should be done here since we've left that for the
    // other function we've defined.
}

bool validate_config(const png2jpeg_settings *config)
{
    // as the name suggests, validate that the structure was filled correctly.
}

void get_in_image_name(char **str, const png2jpeg_settings *config)
{
    // as the name suggests, do all the work necessary to get the input file
    // name.
}

void get_out_image_name(char **str, const png2jpeg_settings *config)
{
    // as the name suggests, do all the work necessary to get the output file
    // name.
}

void convert_png_to_jpeg(png_image *image, const char *in_img_name,
                         const char *out_image_name)
{
    // Here is where all the heavy work is done.
}

int main(int argc, char **argv)
{
    png2jpeg_settings config;
    parse_input_for_config(argc, argv, &config);
    if (!validate_config(&config)) {
        // handle error
    }

    char *in_image_name, *out_image_name;
    get_in_image_name(&in_image_name, &config);
    get_out_image_name(&out_image_name, &config);

    png_image in_image;
    convert_png_to_jpeg(&in_image, in_image_name, out_image_name);
}

This isn't the most thorough example in terms of error checking, implementation, and so forth but it should serve as a good starting point or template for what you'd like to achieve.