0

I am trying to make a photo extraction program stop when it detects it is at the end of the file to be extracted. I did this by placing an if condition:

if (feof(file))
{
   return 2;
}

After a fread function:

fread(array, 1, 512, file);

So that if fread reads to the end of the file, then feof will trigger and thereby end the program. This is my code:

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        printf("Invalid entry.\n");
        return 0;
    }

    int counter = 1;
    FILE* images;
    char jpg_name[8];

    // Check if bytes are jpg. signatures.
    for (int n = 0; counter < 51; n = n + 512)
    {
        // Open file for reading.
        FILE *file = fopen(argv[1], "r");
        if (!file)
        {
            return 1;
        }

        unsigned char array[512];
        fseek(file, n, SEEK_SET);
        fread(array, 1, 512, file); // if EOF, won't have 512 to write into!!!
        if (feof(file))
        {
            return 2;
        }
        fclose(file);

        if (array[0] == 0xff && array[1] == 0xd8 && array[2] == 0xff && (array[3] & 0xf0) == 0xe0)
        {
            // Convert integer to string and store into jpg character array. Increment image number.
            sprintf(jpg_name, "%03i.jpg", counter);
            counter++;

            // Open images file to write into, allocate memory to jpg file to write into, write 512 bytes from array into image file.
            images = fopen(jpg_name, "a");
            fwrite(array, 1, 512, images);
            fclose(images);
        }
        else // If 1st 4 bytes aren't jpg signature.
        {
            if (counter > 1)
            {
                images = fopen(jpg_name, "a");
                fwrite(array, 1, 512, images);
                fclose(images);
            }
        }
    }
}

I have also tried placing the condition:

if (fread(array, 1, 512, file) == 512)

Into the program so that it stops running after it reads fewer than 512 bytes to stop automatically stop the program, but this also doesn't seem to be working.

Any clarification or advice would be much appreciated, thank you!

ckwan
  • 7
  • 1
  • 1
    Please describe the behaviour better than "doesn't work". What is the input file `argv[1]` supposed to be and how does it relate to the other `jpg_name` files being processed? – kaylum Jul 14 '20 at 00:38
  • This is not the purpose of `feof`. After a read operation indicates that the data has been consumed, you use `feof` to determine whether the read operation encountered an error or read all of the data. – William Pursell Jul 14 '20 at 01:39
  • @kaylum Sorry for not being more clear. argv[1] is the name of the file from which I want to extract images from. jpg_name relates to the name of each image I am extracting; I am attempting to number jpg. numerically starting from 1. As there are 50 photos to be extracted, I would like to stop the program after all 50 images have been extracted. Unfortunately, my program is not sure when to terminate, and as such I believe the last photo is being overwritten multiple times, and I am unsure how to fix this. – ckwan Jul 14 '20 at 02:04
  • @WilliamPursell So currently my 'fread' function is reading 512 bytes at a time, once it gets to the end of the file, would it be reading the last 512 bytes of the file (therefore rereading some of the earlier bytes for the last photo more than once), or does it read x many bytes available, with the rest of the array left blank because there are no more bytes to read? – ckwan Jul 14 '20 at 02:07
  • It will only read as many as are available, and `fread` will return the total number read. The array after that will be left unchanged. – William Pursell Jul 14 '20 at 04:14
  • regarding: `fread(array, 1, 512, file); // if EOF, won't have 512 to write into!!! if (feof(file))` Only call `feof()` if the returned value from `fread()` == 0; otherwise if the last of the input file was just read, then `feof()` will return 'true' BUT the last of the read data will not have been processed. – user3629249 Jul 15 '20 at 01:56

1 Answers1

0

Don't open and close the file each time through the loop. Just read the file in blocks of 512 until this reaches EOF.

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

#define SIZE 512

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        printf("Invalid entry.\n");
        return 0;
    }

    int counter = 1;
    FILE* images;
    char jpg_name[8];

    // Open file for reading.
    FILE *file = fopen(argv[1], "r");
    if (!file)
    {
        return 1;
    }

    unsigned char array[SIZE];

    // Check if bytes are jpg. signatures.
    while (fread(array, 1, SIZE, file) == 1)
    {
        if (array[0] == 0xff && array[1] == 0xd8 && array[2] == 0xff && (array[3] & 0xf0) == 0xe0)
        {
            // Convert integer to string and store into jpg character array. Increment image number.
            sprintf(jpg_name, "%03i.jpg", counter);
            counter++;

            // Open images file to write into, allocate memory to jpg file to write into, write 512 bytes from array into image file.
            images = fopen(jpg_name, "a");
            fwrite(array, 1, 512, images);
            fclose(images);
        }
        else // If 1st 4 bytes aren't jpg signature.
        {
            if (counter > 1)
            {
                images = fopen(jpg_name, "a");
                fwrite(array, 1, 512, images);
                fclose(images);
            }
        }
    }
    fclose(file);
}
Barmar
  • 741,623
  • 53
  • 500
  • 612
  • regarding: `while (fread(array, 1, SIZE, file) == 1)` this is not a correct comparison. It is the third parameter value that will be returned when the call is successful. I.E. `while ( fread( array, 1, SIZE, file ) == SIZE )` – user3629249 Jul 15 '20 at 01:58
  • It is a poor idea to be opening/closing the file, over and over and over. Strongly suggest to only use the '4 jpg header' bytes as a reason to (possibly) close a prior output file then open the new file – user3629249 Jul 15 '20 at 02:01
  • @user3629249 I originally moved that out of the loop, then noticed that the filename depends on `counter`. I didn't want to complicate the answer with all the logic needed to determine when to open and close the file. There's not really so much overhead that it's important to fix this. – Barmar Jul 15 '20 at 14:39
  • Actually, opening/closing a file is when an I/O error is most likely to happen. So the code should minimize the number of those events – user3629249 Jul 15 '20 at 14:46
  • @user3629249 Or maybe the opposite, so you can catch the error at the earliest possible time. – Barmar Jul 15 '20 at 14:47
  • If you don't perform a I/O operation, then no I/O error can occur. – user3629249 Jul 15 '20 at 14:50
  • @user3629249 If the disk has failed, you'd like to find out as soon as possible, not wait until much later when you try to write all the buffered data. – Barmar Jul 15 '20 at 14:52
  • the disk (and the data structures in memory) is much more reliable if it doesn't have to have to be modified for each data write with additional open and close operations. Less operations == more reliability – user3629249 Jul 15 '20 at 15:00
  • @user3629249 You're really underestimating the reliability. Unless you're talking about orders of magnitude, this is really not something anyone ever worries about. – Barmar Jul 15 '20 at 15:02