Updated AddFrame method to handle Gif as source

[RyanThiele]

In Issue #8, there is a problem where if the source of the image is an animated gif, the result will be a corrupted animated gif. Due to the nature of animated gifs, the frame only contains what has changed from one frame to the next (similar to interleaved video). By converting the frame to another format, and using that as the source, the result is no longer corrupted.

[DataDink]

Can you update this with a Dispose to the image in the case where it is reassigned during conversion?

[DataDink]

Just to clarify - we don't want to dispose the originally passed in image. Just the new one, if it is created when doing the conversion, after the method is done with it.

[RyanThiele]

The new image has all the properties as the original except for the raw format (e.g. gif, png, bmp, etc...)

// save to memory
img.Save(ms, ImageFormat.Png);
ms.Seek(0, SeekOrigin.Begin);
// pull it back out
img = Image.FromStream(ms);

It's like opening up a the source image (if it is a *.gif) in an image editor and saving it as a *.png. I may be missing something, but I don't see where else the img argument is used in the method other than writing it to the image file stream.

[DataDink]

Right, but when in code you say "img = Image.FromStream(ms);" that creates a new image and the original Image that was passed into the method is no longer referenced by "img". Also the new MemoryStream should probably be disposed as well. Since the "img" reference is being reused for the new image we should make sure to only dispose in the case that the reference has been switched.

Is that incorrect? I just don't want to inadvertently introduce a new memory leak.

[DataDink]

Disregard the part about the new MemoryStream, that is in a using block.

[RyanThiele]

Should I use different naming conventions for the memory stream? The variable ms does feel pretty generic.

Are there any other problems/questions with the PR?

[DataDink]

ms is fine.
I might be missing something in my explanation of the problem. Thanks for bearing with me:

The line where you have "Image.FromStream(ms)" creates a brand new System.Drawing.Image object in memory. It does not discard the previous System.Drawing.Image object that came in with the variable "img". It reassigns the variable "img" to the new memory object. It's fine that the original System.Drawing.Image isn't disposed because that is the responsibility of the method caller and they should decide when it is disposed. What I don't think is okay is the NEW System.Drawing.Image which is created with "Image.FromStream(ms)" is never disposed.

So for example:

using (var ms = new MemoryStream) {
   var img1 = new Bitmap(1,1);
   img1.Save(ms, ImageFormat.Png);
   ms.Seek(0, SeekOrigin.Begin);
   var img2 = Image.FromStream(ms);

   // img1 != img2 (these are not the same object in memory)
   // img2 should be disposed
}

Remember we don't want to dispose the original image that was passed into the method. That is the responsibility of the caller. So we have to make sure not to call img.Dispose() in the case that we do not call "Image.FromStream".

[RyanThiele]

the image passed is is reassigned, so the original reference is destroyed (disposed). Since the argument is passed by reference, I guess it would be better to create a new image and use that in the frame. Originally I wanted to add code that was unobtrusive. PR Updated.