empira / PDFsharp

PDFsharp and MigraDoc Foundation for .NET 6 and .NET Framework
https://docs.pdfsharp.net/
Other
398 stars 91 forks source link

DrawImage draws allways the same (first) image #127

Closed andreas20240621 closed 5 days ago

andreas20240621 commented 2 weeks ago

!!!!! If you think there is a bug in PDFsharp then please use the IssueSubmissionTemplate to make the issue replicable.
!!!!! https://docs.pdfsharp.net/General/IssueReporting.html !!!!! => Sorry, but this link doesn't work

Reporting an Issue Here

When creating an PDF with three different Images, using XGraphics.DrawImage, the resulting PDF schould show up three different images.

Actual Behavior

The resulting pdf shows up the same (first) image for three times. This happend, if PDFSharp 6.1.0 is used This does not happen, if PDFSharp 6.0.0 is used.

Steps to Reproduce the Behavior

!!!!! We strongly recommend using the IssueSubmissionTemplate to make sure we can replicate the issue.
!!!!! https://docs.pdfsharp.net/General/IssueReporting.html !!!!! => sorry, this link does not work.

Download Solution from embedded zip File. Compile Solution using NuGet Package PDFSharp6.1.0 Run the compiled Program. This will produce the wrong result. (file PdfSharpTest1.pdf in the Solution Folder)

Compile Solution using NuGet Package PDFSharp6.0.0 Run the compiled Program. This will produce the correct result. (file PdfSharpTest1.pdf in the Solution Folder)

PdfSharpTest1.zip

This sample project contains 2 PDF Files. These are the results when Project is compiled with PDFSharp 6.0.0 oder PDFSharp 6.1.0 The PDF named PdfSharpTest1-Version6.0.0.pdf contains the expected result. The PDF named PdfSharpTest1-Version6.1.0.pdf contains the wrong result.

ThomasHoevel commented 2 weeks ago

Thanks for the feedback. Issue does not show with Core build and WPF, but shows with GDI build.

Your code that does not work with the GDI build is this:

using FileStream stream = File.OpenRead(iamgeFile);
MemoryStream mems = new MemoryStream();
stream.CopyTo(mems);
mems.Position = 0; // Added by me to make WPF build work.

XImage resultImage = XImage.FromStream(mems);

Here is a shorter version that works:

FileStream stream = File.OpenRead(iamgeFile);
XImage resultImage = XImage.FromStream(stream);

And here is an even shorter version that also works: XImage resultImage = XImage.FromFile(iamgeFile);

Will investigate further next week. It should also work with the indirection to the MemoryStream object.

There was a hyphen missing in the URL: https://docs.pdfsharp.net/General/Issue-Reporting.html

andreas20240621 commented 2 weeks ago

Thanks for your fast response. You are right, that both of your versions do work correctly.

But they both keep an file stream open, what i tried to avoid.

i've added the following "File.Delete" at the end of the Main Routine

pdfoutput.Save(Path.Combine(targetDirectory, "PdfSharpTest1.pdf"));

File.Delete(Path.Combine(targetDirectory, "pic1.jpg"));
File.Delete(Path.Combine(targetDirectory, "pic2.jpg"));
File.Delete(Path.Combine(targetDirectory, "pic3.jpg"));

And the program throw the error System.IO.IOException: "The process cannot access the file 'D:\repos\PdfSharpTest1\pic1.jpg' because it is being used by another process."

In my productiv Application, i do not delete those files of course, but we have PDF outputs with a very large number of images included. So it would be very unhandy to keep them all open, during program execution.

So i will look forward to your investigation next week, to enable MemoryStreams here again. We use Library 6.0.0 for now, so it doesn't hurry.

th-joerger commented 1 week ago

I encountered the same problem in my production environment. Several customers have been severly affected with hundreds of wrongly printed qr codes.

The culprit seems to be this dysfunctional code:

                    else if (image._stream != null!)
                    {
                        if (image._stream is MemoryStream ms)
                        {
                            var md5 = System.Security.Cryptography.MD5.Create();
                            var hash = md5.ComputeHash(ms);
                            image._path = "*md5:" + HashToString(hash);
                        }
                        else
                        {
                            // TODO Use hash code.
                            image._path = "*" + Guid.NewGuid().ToString("B");
                        }
                    }
#if GDI
                    else if (image._gdiImage != null!)
                    {
                        // TODO Use hash code.
                        image._path = "*" + Guid.NewGuid().ToString("B");
                    }
#endif

The md5 is always *md5:d41d8cd98f00b204e9800998ecf8427e which is the md5 hash of an empty input. The stream image._stream is already consumed when computing the hash, because in GDI in XImage.cs, the stream is already used to create an Image.

The work around for GDI with the check for the existence of image._gdiImage comes to late after the md5 hash is already computed.

When the memory stream is reset, everything works as expected.

                        if (image._stream is MemoryStream ms)
                        {
                            ms.Seek(0, SeekOrigin.Begin); // make sure, ms is not already consumed
                            var md5 = System.Security.Cryptography.MD5.Create();
                            var hash = md5.ComputeHash(ms);
                            image._path = "*md5:" + HashToString(hash);
                        }

So, I think the fix is, to check for the existence of image._gdiImage before computing the md5 hash. I send you a PR #128 including a test. I would also appreciate you properly merging PRs instead of simply copying the code and squashing my contribution.

ThomasHoevel commented 5 days ago

The issue has been resolved with version 6.1.1 released today. We are sorry for any inconveniences the issue has caused.

andreas20240621 commented 4 days ago

Yes, the issue has been fixed with version 6.1.1