dilevin / computer-graphics-raster-images

Computer Graphics Assignment about Raster Images
2 stars 11 forks source link

Seg fault in Ubuntu due to default filename #4

Closed Mincelot closed 4 years ago

Mincelot commented 5 years ago

I initially wrote my code in windows and it was able to compile and run perfectly. To double check that my code works, I use a Ubuntu VM to check if my code was able to run under a linux environment, the compilation finished without any error but if I run the program without argument, it produces a seg fault: image And from the output I can see that it crashed when performing the composite step, I also did some verification by edit main.cpp and am sure that the seg fault is caused by the file name array input_filenames. Also if I run the program by entering the file paths manually, it doesn't produce any error: image I'm not very familiar with C++ but this feels like something wrong with the string array declaration in main.cpp? Specifically this section:

  int num_inputs = argc-1;
  if(num_inputs == 0)
  {
    const char * default_input_file_names[] = {
      "../data/dog.png",
      "../data/glasses.png",
      "../data/laser-beams.png",
      "../data/sparkles.png"
    };
    input_filenames = const_cast<char **>(default_input_file_names);
    num_inputs = 4;

Also can we get the environment specification used for marking so we can make sure that our code can run?

rin-23 commented 5 years ago

Can you debug or use printf() statements to check which line gives you seg fault. Your code should compile and run without errors on CDF machines.

Mincelot commented 5 years ago

I edit this section of code in main.cpp from

// Alpha composite multiple images (if present)
  ...
  {
    std::vector<unsigned char> next_rgba;
    int next_height,next_width;
    read_rgba_from_png(input_filenames[f],next_rgba,next_width,next_height);
    ...
  }

To

// Alpha composite multiple images (if present)
  ...
  {
    std::vector<unsigned char> next_rgba;
    int next_height,next_width;
    std::cout << "Reading rgba from png\n";
    read_rgba_from_png(input_filenames[f],next_rgba,next_width,next_height);
    std::cout << "Done\n";
    ...
  }

And running the program shows that seg fault happens while reading rgba from png image

dilevin commented 5 years ago

You should check what the value of f is and what the content of input_filenames is.

ahomayouni commented 5 years ago

I have the same issue on Ubuntu. It seems to be accessing the wrong file. result

rin-23 commented 5 years ago

so the seg fault is happening inside read_rgba_from_png? I don't see a print statement for desaturated.ppm inside read_rgba_from_png where did that come from?

Mincelot commented 5 years ago

I believe the above screen shot was trying to get the value of the filename passed into the function read_rgba_from_png because I also tried to print out the value of input_filenames[0] in main.cpp before calling read_rgba_from_png and it gives the value "desaturated.ppm" and if we try to get the value input_filenames[1] (which is what the for loop under // Alpha composite multiple images (if present) doing), then we get seg fault.

I can confirm this is a problem only with Ubuntu (or maybe with higher version of c++ compiler?) because I can compile and run my code without any error on CDF machine

ahomayouni commented 5 years ago

Yes, I also suspect that it is something low level with passing function pointers to the read_rgba_from_png() function.

Thanks though! I will try on CDF.

after

dilevin commented 5 years ago

You can try using the newest version of stb_image.h available here: https://github.com/nothings/stb

Just to rule out a bug in that code.

dilevin commented 5 years ago

Great! Let's leave this issue open until either I or the TAs make the replacement in the assignment code.

rin-23 commented 5 years ago

yeah i was debugging it and seems like memory of input_filename array was modified if stb_image failed to load.

rin-23 commented 5 years ago

hmm i am still getting seg fault on my end even with the new stb_image

Mincelot commented 5 years ago

Hm, the solution doesn't seems to work in my case, what's the environment that you are running on?

ahomayouni commented 5 years ago

Sorry for the confusion. My last comment was wrong. That was not the fix. The fix for me was actually taking default_input_file_names[] and declaring it outside the if statement (so it's in the same scope of main)

result

rin-23 commented 5 years ago

ok replace the file logic with this:

#include <string>

void get_arguments(int argc, char* argv[], std::vector<std::string>& filenames, int& num_inputs) 
{  
  num_inputs = argc-1;
  if(num_inputs == 0) {
    filenames.push_back("../data/dog.png");
    filenames.push_back("../data/glasses.png");
    filenames.push_back("../data/laser-beams.png");
    filenames.push_back("../data/sparkles.png");
    num_inputs = 4;
  } else {
    for (int i = 1; i < argc; ++i)
        filenames.push_back(argv[i]); 
  }
}

int main(int argc, char *argv[])
{  
  int num_inputs;
  std::vector<std::string> input_filenames;
  get_arguments(argc, argv, input_filenames, num_inputs);

  // read a RGBA .png
  int width,height;
  std::vector<unsigned char> rgba;
  read_rgba_from_png(input_filenames[0],rgba,width,height);
rin-23 commented 5 years ago

@ahomayouni its interesting that it worked with that change. I would suggest to use my solution to be safe.

rin-23 commented 5 years ago

let me know if this works for you @Mincelot @ahomayouni

ahomayouni commented 5 years ago

@rin-23 Yeah! Your solution works on my computer too. Thanks!

Mincelot commented 5 years ago

@rin-23 Yep the solution works, thank you so much! I will keep the issue open until the code in the repo get updated.

WyattLiu commented 5 years ago

emmm, you guys could have just use ./raster + 4 files...

I noticed that default string code was not working, nice pick on the C++ solution with the vector.

We are not submitting main.cpp anyways

WyattLiu commented 5 years ago

A little bit C example that you dont need to delete from the original file.

  char** input_filenames_heap = (char **)malloc(num_inputs * sizeof(char*));

  for(int i = 0; i < num_inputs; i++) {
          size_t size = strlen(input_filenames[i]);
        input_filenames_heap[i] = (char *)malloc(size*sizeof(char));
        memcpy(input_filenames_heap[i],input_filenames[i],size);
  }
  input_filenames = input_filenames_heap;