none
AnimatedPngCreator RRS feed

  • Question

  • Hi,

    I made a small library to create a animated png out of images. It's working well. It can be used like in this example:

    using CMK;
    using System.Collections.Generic;
    using System.Drawing;
    using System.IO;
    
    namespace ApngTest
    {
        class Program
        {
            static void Main(string[] args)
            {
                List<Image> images = new List<Image>
                {
                    Image.FromFile("filename1.bmp"),
                    Image.FromFile("filename2.jpg"),
                    Image.FromFile("filename3.png")
                };
    
                short frameDelay = 1000 / 5; //5 frames per second
                using (FileStream outputFile = File.Create("animated.png"))
                {
                    using (var apngCreator = new AnimatedPngCreator(outputFile, images[0].Width, images[0].Height))
                    {
                        foreach(var image in images)
                        {
                            apngCreator.WriteFrame(image, frameDelay);
                        }
                    }
                }
            }
        }
    }

    The problem is that if the user of this class is not disposing the instance, the created file is corrupted. I think this is ok because it's written in the documentation, but some colleagues said it's not intuitive and this will happen a lot. So what do you think should I do to make it more comfortable to work with it? What is your expection about a "AnimatedPngCretor" class?

    I planed to add a apng editor in the next version (adding and removing images, getting all images out of a apng, edit delay ...)

    I'm happy for every improvement proposal I'm getting. :)

    You can see the whole sourcecode on github:

    https://github.com/cmksoftdev/AnimatedPngCreator

    The name of the package on NuGet is: AnimatedPngCreator

    Greetings, Chris

    PS: You don't have to download or test it. For me it's just important to know what is an user expecting of a nuget package like this.
    • Edited by DerChris88 Tuesday, October 9, 2018 8:00 PM
    • Changed type DerChris88 Wednesday, October 10, 2018 6:03 PM I got all questions answered
    Tuesday, October 9, 2018 7:30 PM

Answers

  • When dealing with IDisposable it is all about ownership. If your creator class owns the output then it is responsible for cleaning it up. If your class doesn't then it isn't your responsibility. This is consistent with how the rest of the framework works. I assume the issue is that your creator is not necessarily writing data to the output stream all the time and in your dispose you are flushing the buffers. This would be consistent with out file IO works. If you open a FileStream and don't dispose of it then it'll eventually get flushed and closed. But during that timeframe the data may or may not be written to disk. This is sort of expected. If code really wants to make sure the data is written then they will flush it. If your creator class has a flush then they could do the same thing.

    I would tend to argue that using IDisposable and using statements should be the low-level/advanced case for those who know what they are doing. For the common cases you should provide helper methods that wrap this logic internally. As an example, reading/writing an entire file is a common thing to do so instead of having to work with FileStream and using the File class provides helper methods that do this for you. In your case I'd say identify the common use cases and provide (static) helper classes that do all this logic for the user. Then they can replace the using/stream logic with a simple method call.

    Given your very specific sample as an example I'd create a static method that accepts IEnumerable<Image> (which by the way needs to be disposed) and an output file name and does the logic in your code automatically. Perhaps another helper could accept a series of file paths (if this is common).


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by DerChris88 Wednesday, October 10, 2018 6:03 PM
    Tuesday, October 9, 2018 7:59 PM
    Moderator
  • For a time component you could do it a couple of different ways. I would start with an overload that accepts the list of files and the time delay as a TimeSpan. If there is a reasonable default then I might provide an overload that assumes that. If more options are going to be needed later then I'd go ahead and introduce a Options type that specifies the default delay and other options and have an overload for that. Ultimately the options would generally match the properties on a creator instance.

    If each file needs its own delay then I think adding a custom type to wrap that makes the most sense (e.g Frame). You could use a tuple but in my opinion tuple support across .NET is not fully integrated yet. You still need a NuGet package for older versions and I personally find tuples to be really for internal code. You aren't really making your code easier to use by using tuples, you're just making it easier to write and change (not features you really need in a public interface). At this point I'm also not seeing tuples being used in the public interfaces for any major framework so I would lean toward a simple custom type.

    But it would seem odd to me to have a type that accepts a filename and delay; and then another for (perhaps) a stream and delay. So I would probably need to rethink the approach after I played around with it a little bit. Perhaps at the point where you need multiple files with different delays then a specialized collection type would be better. Your collection type would have methods to add files with their delays, and streams and their delays, etc. Then the collection type is a parameter to your methods. This would allow your helper method to enumerate and get the information for each file and yet allow you to extend what it supports (filenames, images, streams, etc) just by adding methods. Again though I'd have to code it up and then play around with it to see how it feels.


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by DerChris88 Wednesday, October 10, 2018 6:03 PM
    Wednesday, October 10, 2018 2:34 AM
    Moderator
  • Greetings Chris.

    I believe your colleagues are correct. Your library should take care of any disposing, so that users don't have to worry about it.

    In fact, if I were using such a library, I would want (and expect) it to work something like this.

    namespace ApngTest
    {
        class Program
        {
            static void Main(string[] args)
            {
                List<Image> images = new List<Image>
                {
                    Image.FromFile("filename1.bmp"),
                    Image.FromFile("filename2.jpg"),
                    Image.FromFile("filename3.png")
                };
    
                short frameDelay = 1000 / 5; //5 frames per second
                AnimatedPngCreator.Create("animated.png", images, frameDelay);
                
            }
        }
    }

    So there is a static method called Create which takes as its arguments the filename to create, a list of images (possibly as IEnumerable), and the delay between frames. In this case the height and width could be deduced from the images, but another overload could be added where the user passes in a different height and width if required.

    The method would then be responsible for opening and closing the file and performing any disposes to ensure the file is not corrupted. The user would not have to worry about it.

    The only potential problem with this scenario is if the user requires different delays for different frames. To take care of this, you could provide another class which has properties of an image and a delay, and write an overload of Create which takes an IEnumerable of that class rather than of Image.


    • Edited by Ante Meridian Wednesday, October 10, 2018 3:18 AM too many braces in the code
    • Marked as answer by DerChris88 Wednesday, October 10, 2018 6:03 PM
    Wednesday, October 10, 2018 3:17 AM

All replies

  • When dealing with IDisposable it is all about ownership. If your creator class owns the output then it is responsible for cleaning it up. If your class doesn't then it isn't your responsibility. This is consistent with how the rest of the framework works. I assume the issue is that your creator is not necessarily writing data to the output stream all the time and in your dispose you are flushing the buffers. This would be consistent with out file IO works. If you open a FileStream and don't dispose of it then it'll eventually get flushed and closed. But during that timeframe the data may or may not be written to disk. This is sort of expected. If code really wants to make sure the data is written then they will flush it. If your creator class has a flush then they could do the same thing.

    I would tend to argue that using IDisposable and using statements should be the low-level/advanced case for those who know what they are doing. For the common cases you should provide helper methods that wrap this logic internally. As an example, reading/writing an entire file is a common thing to do so instead of having to work with FileStream and using the File class provides helper methods that do this for you. In your case I'd say identify the common use cases and provide (static) helper classes that do all this logic for the user. Then they can replace the using/stream logic with a simple method call.

    Given your very specific sample as an example I'd create a static method that accepts IEnumerable<Image> (which by the way needs to be disposed) and an output file name and does the logic in your code automatically. Perhaps another helper could accept a series of file paths (if this is common).


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by DerChris88 Wednesday, October 10, 2018 6:03 PM
    Tuesday, October 9, 2018 7:59 PM
    Moderator
  • Hi Michael Taylor,

    thank you for your post and thank you for the good ideas. :)

    You giving me a lot of tasks I have to add now to the project:

    • adding static helper methods
    • using file paths as parameter instead of adding single images
    • using file path for creating output file
    • using IEnumerable<Image>

    By using the IEnumerable I have a question: How should I do it with setting the time delay? Should I use a IEnumerable<tuple<Image, int>>, or a own DTO class like Frame containing Image and int? Or is there something more intuitive? By using IEnumerable without a time delay value my class could just use a default value for this. Telling the user to set it afterwards by an index is not good, I think. Do you have some ideas for this, too?

    My appreciation for your help!

    Greetings, Chris



    • Edited by DerChris88 Tuesday, October 9, 2018 8:28 PM
    Tuesday, October 9, 2018 8:25 PM
  • For a time component you could do it a couple of different ways. I would start with an overload that accepts the list of files and the time delay as a TimeSpan. If there is a reasonable default then I might provide an overload that assumes that. If more options are going to be needed later then I'd go ahead and introduce a Options type that specifies the default delay and other options and have an overload for that. Ultimately the options would generally match the properties on a creator instance.

    If each file needs its own delay then I think adding a custom type to wrap that makes the most sense (e.g Frame). You could use a tuple but in my opinion tuple support across .NET is not fully integrated yet. You still need a NuGet package for older versions and I personally find tuples to be really for internal code. You aren't really making your code easier to use by using tuples, you're just making it easier to write and change (not features you really need in a public interface). At this point I'm also not seeing tuples being used in the public interfaces for any major framework so I would lean toward a simple custom type.

    But it would seem odd to me to have a type that accepts a filename and delay; and then another for (perhaps) a stream and delay. So I would probably need to rethink the approach after I played around with it a little bit. Perhaps at the point where you need multiple files with different delays then a specialized collection type would be better. Your collection type would have methods to add files with their delays, and streams and their delays, etc. Then the collection type is a parameter to your methods. This would allow your helper method to enumerate and get the information for each file and yet allow you to extend what it supports (filenames, images, streams, etc) just by adding methods. Again though I'd have to code it up and then play around with it to see how it feels.


    Michael Taylor http://www.michaeltaylorp3.net

    • Marked as answer by DerChris88 Wednesday, October 10, 2018 6:03 PM
    Wednesday, October 10, 2018 2:34 AM
    Moderator
  • Greetings Chris.

    I believe your colleagues are correct. Your library should take care of any disposing, so that users don't have to worry about it.

    In fact, if I were using such a library, I would want (and expect) it to work something like this.

    namespace ApngTest
    {
        class Program
        {
            static void Main(string[] args)
            {
                List<Image> images = new List<Image>
                {
                    Image.FromFile("filename1.bmp"),
                    Image.FromFile("filename2.jpg"),
                    Image.FromFile("filename3.png")
                };
    
                short frameDelay = 1000 / 5; //5 frames per second
                AnimatedPngCreator.Create("animated.png", images, frameDelay);
                
            }
        }
    }

    So there is a static method called Create which takes as its arguments the filename to create, a list of images (possibly as IEnumerable), and the delay between frames. In this case the height and width could be deduced from the images, but another overload could be added where the user passes in a different height and width if required.

    The method would then be responsible for opening and closing the file and performing any disposes to ensure the file is not corrupted. The user would not have to worry about it.

    The only potential problem with this scenario is if the user requires different delays for different frames. To take care of this, you could provide another class which has properties of an image and a delay, and write an overload of Create which takes an IEnumerable of that class rather than of Image.


    • Edited by Ante Meridian Wednesday, October 10, 2018 3:18 AM too many braces in the code
    • Marked as answer by DerChris88 Wednesday, October 10, 2018 6:03 PM
    Wednesday, October 10, 2018 3:17 AM
  • Hi Michael,

    thank you so much. This is giving me an exact idea how I should implement the whole framework.

    Now I have got work to do... ;D

    My appreciation for your help!

    Greetings, Chris

    Wednesday, October 10, 2018 5:59 PM
  • Hi Ante,

    thank you very much! This is helping me a lot. I will implement the framework that your code should work.

    My appreciation for your help!

    Greetings, Chris

    PS: I think that's all I need to know right now. So I will change the type and mark Michael's and your post as answer. :)

    Wednesday, October 10, 2018 6:02 PM
  • Hi Michael and Ante,

    thank you very much :) ! I made it:

    // Example 1:
    var filePaths = new List<string>
    {
        "1.bmp", "2.bmp", "3.bmp"
    };
    CMK.AnimatedPngCreator.Create("out.png", filePaths, 1000);
    
    // Example 2:
    var images = new List<Image>
    {
        Image.FromFile("1.bmp"),
        Image.FromFile("2.bmp"),
        Image.FromFile("3.bmp")
    };
    CMK.AnimatedPngCreator.Create("out.png", images, 1000);
    
    // Example 3:
    var frames = new List<Frame>
    {
        CMK.AnimatedPngCreator.Frame(Image.FromFile("1.bmp"), 1000),
        CMK.AnimatedPngCreator.Frame(Image.FromFile("2.bmp"), 1000),
        CMK.AnimatedPngCreator.Frame(Image.FromFile("3.bmp"), 1000)
    };
    CMK.AnimatedPngCreator.Create("out.png", frames);

    I think this is how you explained it.

    Greetings, Chris

    Friday, October 12, 2018 7:16 PM