0

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");
    }
  }
}
3

[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. ;-)

3
  • Thanks for the advice, but I had 512 in the place of sizeof for most of the time making program and I have switched back to it and it makes no difference, that I can see. Also I addressed the missing buffer[3] code in my question, the problem is that I can't find the signature in the first bytes.
    – stabilo
    Jul 12 '17 at 9:36
  • @Cliff I agree with your BUFFSIZE advice, IMO it makes the code more readable and maintainable. Since buffer is a BYTE array, not a pointer, sizeof(buffer) is (in this case) 512. Jul 14 '17 at 14:13
  • I stand corrected and will remove the notation.
    – Cliff B
    Jul 14 '17 at 15:56
0

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?

2
  • Opening the output file in append mode is still a problem. If the file doesn't exist, it'll work. IF it exists, then anything will be appended to the file. So, with repeated executions, the program will simply append to the existing file. If the program is otherwise correct, the header info will provide the correct info to display the image, but there will be multiple copies of the file contents in the file. It will be several times larger than needed.
    – Cliff B
    Jul 11 '17 at 23:55
  • Oh that is pretty interesting. I didn't think it would have that behavior. Is not putting the "&" in front of the buffer a problem? I would have thought it wouldn't work at all in that case, but maybe I don't understand addresses as well as I thought... Jul 12 '17 at 0:11

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .