PSET4 RECOVER Can't Open JPEGS, I Have Tried Everything Please HELP!

Question

so this is my second time posting about this pset, my code is below. No images will open, with the current code. However when I do get something to open it is just 1 file and it says invalid or unsupported format. When run, the program cannot pass the 'if' condition that checks for the 4 byte signature, that is where it gets stuck, and to me it suggests that it can't find the signatures in the files.

I just can't see what I have done wrong, please give me some advice, what am I missing, what am I not seeing??

Also about my code, old is the card.raw and "new" is the images being produced, well in theory. Also for my 'if' condition, after the while loop, where I look for the first 4 byte signature; I know that the spec call for 4 bytes but it seems more likely to pass with less conditions and so only the first 3 bytes are there at the moment, however I have tried with all 4 and still no result! x

#include <cs50.h>
#include <stdio.h>
#include <string.h>
#include <stdint.h>


int main(int argc, char* argv[])
{
// ensure only 1 command line argument
if (argc != 2)
{
    fprintf(stderr, "Usage: ./recover image\n");
    return 1;
}

// declare variable
char new_files[8];
int JPEG_Counter = 0;
FILE* new = NULL;


// open memory card file & new outfile
FILE* old = fopen(argv[1], "r");

// check the fill isnt empty
if (old == NULL)
{
    fclose(old);
    fprintf(stderr, "fopen() error, retry\n");
    return 2;
}


// confirm fopen() worked
eprintf("File open sucsessfully\n");


// create buffer
typedef uint8_t BYTE;
BYTE buffer[512] = {0};


// read old file into buffer
while (fread(&buffer, sizeof(buffer), 1, old) != 0)
{

// confirm fread() worked
eprintf("File read sucsessfully\n");


    if (buffer[0] == 0xFF && 
    buffer[1] == 0xD8 && 
    buffer[2] == 0xFF)
    {

        sprintf(new_files, "%03d.jpg", JPEG_Counter);
        JPEG_Counter++;
        eprintf("Counter has reached: %d\n", JPEG_Counter);


        // open new files with naming system
        new = fopen(new_files, "a");

        // write into new from buffer
        fwrite(buffer, sizeof(buffer), 1, new);

        // confirm fwrite() worked
        eprintf("File wrote sucsessfully\n");


            // close all files
            if (feof(old))
            {
                fclose(old);
            }

            else if (feof(old))
            {
                fclose(new);
            }

        // confirm fclose() worked
        eprintf("File closed sucsessfully\n");

        //sucsess 
        return 0;
    }                        

    else
    {
        fclose(old);
        eprintf("Files do not have first 3 byte signitures\n");
    }
  }
}

Answer 1

[EDIT: followup to comments]

The code never has a chance to get to a signature. There exists more than one block of garbage data at the front of the file, so the while loop needs to run a few times before it finds a signature. However, if the signature isn't detected immediately, the else clause inside the while loop will close the input file. When the loop returns to the while statement, the read fails and the loop exits. In short, the code is reading one block of code and exiting.

There are more issues. If the code does succeed in finding a signature, it's going to run into a return 0; statement, which will terminate the program immediately on execution. Again, the program is short-circuited.

There also needs to be a check for the 4th byte of the signature and also the matter of opening the file in write vs. append mode. And it would make the code more readable and efficient to use a constant like #define BUFFSIZE 512 in place of calculating sizeof(buffer) in the fread statements.

Finally, there's no code at all to handle the blocks of data between signatures - the rest of each file.

There may be further issues, but these are significant and will require more than simple changes to implement. My recommendation is to pursue these issues, and if you have new problems, open a new question.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)

Answer 2

First off, nice progress! Your code has come a long way.

To add to Cliff B's answer I'd like to point out a couple more problem spots.

You don't want to close the file if the 512 bytes in the buffer does not contain the start of a jpeg because a jpeg can be multiple 512 byte sections, so it would close whenever a jpeg is longer than 512 bytes (as the next section of the jpeg would not contain the special starting sequence).

What you would want to do if you do not encounter the special sequence of is still write it to the same image, the only catch is that if you haven't encountered the first jpeg (meaning that there is blank space at the beginning of the file which is entirely possible) to ignore that 512 bytes, so you probably want to be able to check if this is your first jpeg encountered.

You do however want to close the old file at the end since you are done with it.

Interestingly I would have thought that not including &buffer instead of just buffer would cause issues, but when I tried it in my own code it worked fine. Also I would have thought that you need to use fwrite with "w" instead of "a" but I tried that and it worked fine too. I guess "a" would pretty much do the same thing, but I really thought that you need to write at the adress of the buffer.

Do you know why writing at the buffer works?