image recovery program has an issue

Question

Well basically I wrote this program for my computer course (shoutout CS50) that recovers images from a .raw file. I have managed to have the program recover 48 of the 50 files in that file.

The issue im having right now with the program is that the program cannot recover both the first and the second file located on .raw. It either reads and writes the very first file (this girl in a snowy background) or the second file on the .raw (guy behind books).

For some reason if I change fopen from write to append I can switch between the photo of the girl and the guy, but I cant seem to be able to open both.

https://github.com/CoreData/cs50/blob/master/pset4/jpg/card.raw This is the link to card.raw, unfortunately its not the same one that Im using but even using this one you get two different images for image1.jpg depending on whether you have fopen with an "a" or "w".

Any ideas???

if you guys want any additional info just let me know

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

#include "bmp2.h"

int main(void)
{
    /*OPEN CARD FILE*/
    char* infile = "card.raw";;

    FILE* card = fopen(infile, "r");
    if (card == NULL)
    {
        printf("Could not open %s.\n", "card.raw");
        return 2;
    }

    int f = 0, c = 0, l = 0, x = 128, imageno = 1;
    // c signals that a jpg is being written
    // l size control, 0 means 0 jpgs
    FILE* images;
    char* title = (char*)malloc(15);

    /*repeat until end of card*/
    do
    {
        //read one block into buffer
        INTROJPG *buffer = (INTROJPG*)malloc(sizeof(INTROJPG)*x);

        for (int i = 0; i < 128; i++)
        {
            fread(&buffer[i], sizeof(INTROJPG), 1, card);
        }

        if (buffer[0].first == 0xff && buffer[0].second == 0xd8 && buffer[0].third == 0xff)
        {
            sprintf(title, "image%d.jpg", imageno); //change jpg title

            if (f == 1) //close previous jpg
            {
                fclose(images);
                imageno++;
            }

            images = fopen(title, "w");
            f = 1; //very first jpg has been opened
            c = 1; //jpg open
            l++; //jpg count + 1
        }

        //jpg already open?
        if (c == 1)
        {
            for (int i = 0; i < 128; i++)
            {
                fwrite(&buffer[i], sizeof(INTROJPG), 1, images);
            }
        }
        free(buffer);
    }    
    while (l < 50);
    free(title);
    return 5;
        //close any remaining files
}


and this is my bmp2.h file

    #include <stdint.h>

/**
 * Common Data Types 
 *
 * The data types in this section are essentially aliases for C/C++ 
 * primitive data types.
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/cc230309.aspx.
 * See http://en.wikipedia.org/wiki/Stdint.h for more on stdint.h.
 */
typedef uint8_t  BYTE;
typedef uint32_t DWORD;
typedef int32_t  LONG;
typedef uint16_t WORD;

/**
 * BITMAPFILEHEADER
 *
 * The BITMAPFILEHEADER structure contains information about the type, size,
 * and layout of a file that contains a DIB [device-independent bitmap].
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/dd183374(VS.85).aspx.
 */
typedef struct 
{ 
    WORD   bfType; 
    DWORD  bfSize; 
    WORD   bfReserved1; 
    WORD   bfReserved2; 
    DWORD  bfOffBits; 
} __attribute__((__packed__)) 
BITMAPFILEHEADER; 

/**
 * BITMAPINFOHEADER
 *
 * The BITMAPINFOHEADER structure contains information about the 
 * dimensions and color format of a DIB [device-independent bitmap].
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/dd183376(VS.85).aspx.
 */
typedef struct
{
    DWORD  biSize; 
    LONG   biWidth; 
    LONG   biHeight; 
    WORD   biPlanes; 
    WORD   biBitCount; 
    DWORD  biCompression; 
    DWORD  biSizeImage; 
    LONG   biXPelsPerMeter; 
    LONG   biYPelsPerMeter; 
    DWORD  biClrUsed; 
    DWORD  biClrImportant; 
} __attribute__((__packed__))
BITMAPINFOHEADER; 

/**
 * RGBTRIPLE
 *
 * This structure describes a color consisting of relative intensities of
 * red, green, and blue.
 *
 * Adapted from http://msdn.microsoft.com/en-us/library/aa922590.aspx.
 */
typedef struct
{
    BYTE  rgbtBlue;
    BYTE  rgbtGreen;
    BYTE  rgbtRed;
} __attribute__((__packed__))
RGBTRIPLE;


typedef struct
{
    BYTE first;
    BYTE second;
    BYTE third;
    BYTE fourth;
} __attribute__((__packed__))
INTROJPG;

typedef struct
{
    BYTE image;
}
BYTEIMAGE;

Answer 1

First things first, I'll try to improve a few things in your code. I've also done this pset and it is nice to help others.

INTROJPG *buffer = (INTROJPG*)malloc(sizeof(INTROJPG)*x);

At this part, you know that the size of both INTROJPG and x are constant, so there is no need to constantly allocate and free memory at every iteration, that takes much more time than simply creating a normal array. Also, why is the buffer a pointer to INTROJPG? If it is only to test for a header at each iteration, I don't think it is worth it, you could simply access the first 4 bytes of a normal BYTE array.

I'd create a static array of 512 BYTEs (the struct on the library), because this is the size you are constantly allocating and freeing and also you are using BYTEs, not INTROJPGs.

Second, at this section and another similar one:

for (int i = 0; i < 128; i++)
{
    fread(&buffer[i], sizeof(INTROJPG), 1, card);
}

There is absolutely no need for this loop or, again, even using INTROJPG. You are always reading and writing 512 bytes, you could use:

fread(buffer, 4, 128, card);
// or even better
fread(buffer, 512, 1, card);

Now about your problem, I've tested your code (without any modifications) multiple times and found nothing wrong with image1.jpg and image2.jpg. Yes, I changed "w" mode to "a" and vice-versa.

However, your code is faulty in regard to the last image, your last image is image49.jpg, when it should be image50.jpg, and your image49.jpg does not even open, and that's because the loop is finished before the rest of image49.jpg is stored, i.e., you are storing only the first 512 bytes of image49.jpg.

To fix that, I've changed the condition of the do-while loop to keep going until the end of the card file, IIRC the problem guarantees the last block being part of the last image or something like that, if not, it's up to you to fix this little problem!

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

#include "bmp2.h"

int main(void)
{
    /*OPEN CARD FILE*/
    char* infile = "card.raw";;

    FILE* card = fopen(infile, "r");
    if (card == NULL)
    {
        printf("Could not open %s.\n", "card.raw");
        return 2;
    }

    int f = 0, c = 0, imageno = 1;
    // c signals that a jpg is being written
    // l size control, 0 means 0 jpgs
    FILE* images;
    char title[25];
    BYTE buffer[512];

    /*repeat until end of card*/
    do
    {

        fread(buffer,  512, 1, card);

        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff)
        {
            sprintf(title, "image%d.jpg", imageno); //change jpg title

            if (f == 1) //close previous jpg
            {
                fclose(images);
                imageno++;
            }

            images = fopen(title, "w");
            f = 1; //very first jpg has been opened
            c = 1; //jpg open
        }

        //jpg already open?
        if (c == 1) fwrite(buffer, 512, 1, images);
    }    
    while (!feof(card));
    return 5;
        //close any remaining files
}

One last thing, why are you returning 5 at the end of the program? Just curious.