Loading BitmapImages out of a Zip file

Question

I'm working on a personal project to get better with C#, and as part of that I have a zip file with 20 or so images saved inside. I want to load those images out of the zip file and display them in my WPF app. I have it working, but I don't know if what I have is the best way to go about this. The goal is to load the images to be used as fast as possible, so I tried using threads, which work, but again I'm not sure if this is the proper way to utilize them for this, or if threads are even the best way to achieve the quickest time for the task.

I would appreciate any feedback anyone could give to help me improve this code and write better code in the future.

In the code below, I have the two functions I use to open and get the images. the variable ChapterLocation is a string that stores the file location of the .zip file, and Pages is a List. Page is a custom struct that just holds a BitmapImage and a String that gives that image a name that I can display.

private void BitmapLoad(ZipArchiveEntry z)
{
    using (var stream = z.Open())
    {
        using (var mstream = new MemoryStream())
        {
            stream.CopyTo(mstream);
            mstream.Position = 0;

            var bitmap = new BitmapImage();
            bitmap.BeginInit();
            bitmap.CacheOption = BitmapCacheOption.OnLoad;
            bitmap.StreamSource = mstream;

            bitmap.EndInit();
            bitmap.Freeze();

            Pages.Add(new Page(bitmap, z.Name));
        }
    }
}

private void LoadChapterOptimized()
{
    var zipFile = ZipFile.OpenRead(ChapterLocation);
    foreach (ZipArchiveEntry z in zipFile.Entries)
    {
        Thread t = new Thread(() => BitmapLoad(z));
        t.Start();
        t.Join();
        Debug.WriteLine(z.Name);
    }
    Pages.Sort();
    zipFile.Dispose();
}

Answer 1

    var zipFile = ZipFile.OpenRead(ChapterLocation);
    ...
    zipFile.Dispose();

One word: using.

---

    using (var stream = z.Open())
    {
        using (var mstream = new MemoryStream())
        {
            stream.CopyTo(mstream);
            mstream.Position = 0;

            var bitmap = new BitmapImage();
            bitmap.BeginInit();
            bitmap.CacheOption = BitmapCacheOption.OnLoad;
            bitmap.StreamSource = mstream;

            bitmap.EndInit();
            bitmap.Freeze();

This needs a comment explaining why mstream is necessary and you can't just use stream.

I suspect that really mstream is either unnecessary or insufficient. If it's necessary then it's probably because of threading issues due to multiple ZipArchiveEntry streams all reading from one backing stream, and in that case doing the read with stream.CopyTo might be reducing the probability of the problem surfacing rather than actually fixing it.

---

    foreach (ZipArchiveEntry z in zipFile.Entries)
    {
        Thread t = new Thread(() => BitmapLoad(z));
        t.Start();
        t.Join();
        Debug.WriteLine(z.Name);
    }

Ah, it's not really multi-threaded after all. If you want to do old-school multithreading then you need to spin off all of the threads before calling Join().

I say "old-school" because a more modern approach would be to use Parallel Linq. Bearing in mind my previous comment about streams, the approach would probably be to map the entries to (string Name, MemoryStream ms)s in one thread and then map those tuples to Pages in parallel threads. I think that this can be written as

Pages = zipFile.Entries.
    Select(z => Read(z)).
    AsParallel().
    Select(tuple => LoadPage(tuple)).
    OrderBy(page => page).
    ToList();

but I haven't really used Parallel Linq myself, and I don't guarantee that this does the Read calls all in the same thread.