0

Getting closer on this now and am no longer getting a segmentation fault. However, my Jpeg images created by the program are not correct. I think part of the problem is that I am writing rather than appending to existing Jpegs. Any ideas?

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

int main(int argc, char *argv[])
{
    // Check to make sure user submits just 1 command-line argument
    if (argc != 2)
    {
        return 1;
    }

    // Define file pointer and open card.raw file entered by user
    FILE *fptr = fopen(argv[1], "r");
    if (!fptr)
    {
        return 1;
    }

    // Counter for number of jpegs
    int ctr = 0;

    // Open space in memory to read bytes to
    unsigned char bytes[512];

    // Open space in memory for new jpeg filenames
    char filename[4];

    // Create pointer for new jpeg files
    FILE *img = NULL;

    do
    {
        // Read 512 bytes from card.raw file
        fread(bytes, 1, 512, fptr);

        // Look for the start of jpeg files
        if (bytes[0] == 0xff && bytes[1] == 0xd8 && bytes[2] == 0xff && (bytes[3] & 0xf0) == 0xe0)
        {
            // If this is the first jpeg
            if (ctr == 0)
            {
                // Create new filename for jpeg file
                sprintf(filename, "%03i.jpg", ctr);

                // Open new file to hold jpeg
                img = fopen(filename, "w");

                // Write data from buffer to file
                fwrite(bytes, 1, 512, img);

                // Add to counter
                ctr = ctr + 1;
            }

            // If this is not the first jpeg
            else
            {
                // Close previous jpeg
                fclose (img);

                // Create new filename for jpeg file
                sprintf(filename, "%03i.jpg", ctr);

                // Open new file to hold jpeg
                img = fopen(filename, "w");

                // Write data from buffer to file
                fwrite(bytes, 1, 512, img);

                // Add to counter
                ctr = ctr + 1;
            }
        }

        // If not the start of a new jpeg
        if (img != NULL)
        {
            // If before the first jpeg in the file
            if (ctr == 0)
            {
                continue;
            }

            // If need to add on to an existing jpeg that is already open
            if (ctr > 0)
            {
                // Write data from buffer to file
                fwrite(bytes, 1, 512, img);
            }
        }
    }
    while (!feof(fptr));

    fclose(img);

    // Close card.raw file
    fclose(fptr);
}

*** ORIGINAL QUESTION***

Still having some challenges with this program. Right now it is creating a segmentation fault. Where am I going wrong?

 #include <stdio.h>

#include <stdlib.h>
#include <cs50.h>

int main(int argc, char *argv[])
{
    // Check to make sure user submits just 1 command-line argument
    if (argc != 2)
    {
        return 1;
    }

    // Define file pointer and open file
    FILE *fptr = fopen(argv[1], "r");
    if (!fptr)
    {
        return 1;
    }

    // Counter for number of jpegs
    int ctr = 0;

    while (!feof(fptr))
    {
        // Open space in memory to read bytes to
        unsigned char bytes[512];

        // Read 512 bytes from card.raw file
        fread(bytes, 1, 512, fptr);

        string filename = NULL;
        
        // Process for if haven't found the start of the first JPEG yet
        if (bytes[0] != 0xff && bytes[1] != 0xd8 && bytes[2] != 0xff && (bytes[3] & 0xf0) != 0xe0 && ctr == 0)
        {
            continue; 
        }

        // Process for if the first 4 bytes are the start of a new JPEG
        if (bytes[0] == 0xff && bytes[1] == 0xd8 && bytes[2] == 0xff && (bytes[3] & 0xf0) == 0xe0)
        {
            // Create new filename for jpeg file
            sprintf(filename, "%03i.jpg", ctr);

            // Open new file to hold jpeg
            FILE *img = fopen(filename, "w");

            // Write data from buffer to file
            fwrite(bytes, 1, 512, img);

            // Add to counter
            ctr = ctr + 1;

            // Close new JPEG file
            fclose(img);
        }

        // Process for if not the start of a new JPEG
        if (bytes[0] != 0xff && bytes[1] != 0xd8 && bytes[2] != 0xff && (bytes[3] & 0xf0) != 0xe0 & ctr >= 1)
        {
            // Open file to hold jpeg
            FILE *img = fopen(filename, "a");

            // Write data from buffer to file
            fwrite(bytes, 1, 512, img);

            // Close JPEG file
            fclose(img);
        }
    }

    // Close file
    fclose(fptr);
}
0

There are a few problems to fix. First, there's this:

    string filename = NULL;

This creates a string of length 0. When a string is created, its size (i.e., the physical memory allocated to it) cannot be changed. This initialization doesn't allocate any memory for it. Instead, the code should set the size for the filename. We already know how many chars should be allocated, plus 1 for the end of string marker, '\0'.

Next is this:

if (bytes[0] != 0xff && bytes[1] != 0xd8 && bytes[2] != 0xff && (bytes[3] & 0xf0) != 0xe0 && ctr ...)

This doesn't do what you think. Simply changing the "==" condition to "!=" isn't the same as looking for the negative of all "==" tests. There are entire college courses that delve into this. The short version is that if one or more bytes are equal but not all 4 bytes, this test goes off the rails.

When trying to get the exact opposite of a complex test, the simple and correct way to do it is to apply the not operator to the entire test as a whole.

if (!(bytes[0] != 0xff && bytes[1] != 0xd8 && bytes[2] != 0xff 
      && (bytes[3] & 0xf0) != 0xe0) 
      && ctr == 0)

Note that the ctr test is outside the parentheses that the ! operator acts on.

Next, the fread is inside the loop. That means that the code will attempt to read a block of 512 bytes, will process the result, and only then, will come back to the beginning of the loop to test for EOF. When the read actually encounters the EOF, the read fails, the buffer retains the previous loop's data, and will write it out to the output file, thus adding 512 bytes to the last file. Consider how you could incorporate the fread into the while loop setup statement.

Why does the code close and reopen the output files repeatedly for each data block? Opening and closing files takes a tremendous amount of time and overhead, especially with spinning hard drives (up to 1000x the time taken for a memory read.) The files should simply be opened for write when a signature block is found and closed when the next signature is found, before opening the next file. There's no need to close and reopen files like this.

Finally, you should spend some time thinking about how to simplify code. For instance, the test for whether to write to a file could focus on whether any output files have been opened. It's a lot simpler than checking for a not-signature.

The garbage at the front of the file can be handled by later code that looks for a signature or checks whether a file has been opened. The first if/continue code block isn't really necessary, if you streamline the code some more.

There may be more issues, but these are the main ones. Other issues may disappear or may manifest, bases on what changes you make after reading this.

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

3
  • Thanks for your help on this. I am getting closer and now am recovering 50 jpegs! They are just corrupted haha. But getting closer. How do you test for if a file is open or not? I see why that would simplify the code. I did some research on this one but couldn't figure it out. – getsendy Sep 28 '20 at 23:32
  • It's simple in this program. When you declare the file pointer, initialize it to NULL. Then, if (filepointer == NULL) then it hasn't been opened. – Cliff B Sep 29 '20 at 6:21
  • thankst! Just published updated code above. Getting closer but still not working, I think due to a file write vs. append issue. Any suggestions? – getsendy Sep 30 '20 at 20:37

You must log in to answer this question.

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