search

ulrichard / ftgl

fork from http://sourceforge.net/projects/ftgl
Other
56 stars 44 forks source link

Fixes for Debian bugs 531489, 589601, 742469, 760571 #9

Closed frankheckenbach closed 6 years ago

frankheckenbach commented 6 years ago

see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=760571;msg=15

ulrichard commented 6 years ago

Hi Frank,

thanks a lot for your contribution to ftgl. I did a brief review, and I will merge it. But I don't feel, that I have any authority over this library. The last time I used it was a couple of years ago. A long time ago, I added some code to be able to extract the geometry of the generated text for use in WebGL. Because I couldn't figure out how to contribute my code to the original project, I created this fork on GitHub. Since you are actively using and enhancing it, it might be more appropriate, if you run the authoritative repo. If that's fine with you, I will add a note.

frankheckenbach commented 6 years ago

Hi Richard,

thanks a lot for your contribution to ftgl. I did a brief review, and I will merge it.

Thanks!

But I don't feel, that I have any authority over this library. The last time I used it was a couple of years ago. A long time ago, I added some code to be able to extract the geometry of the generated text for use in WebGL. Because I couldn't figure out how to contribute my code to the original project, I created this fork on GitHub. Since you are actively using and enhancing it, it might be more appropriate, if you run the authoritative repo. If that's fine with you, I will add a note.

"Actively using and enhancing" is perhaps an overstatement. My changes are just some bug fixes (most of them also a long time ago). Though I'm still using it in my projects.

For practical purposes, it doesn't matter much at the moment, since both repos are identical now. But if you want to get "rid" of it, I have no objections.

What's more important to me is to get a fixed version into Debian (and hopefully other distros, but I use Debian), so I hope you don't mind I'm including Manuel A. Fernandez Montecelo in this email.

Manuel, AFAICS the code in the repo is now ready. I've tested it with my code and looked through the changes compared to the version in Debian now, and found no regressions.

BTW, my repo it at https://github.com/frankheckenbach/ftgl -- as said above, it's currently identical to Richard's.

All it needs is some formalities like a version tag etc., which you as a Debian maintainer probably know more about than me. So I hope you can make a Debian upload from it, or is there anything else you need? Should we ask the original developers for their opinion?

Regards, Frank

frankheckenbach commented 6 years ago

OlivierSohn commented on this pull request.

I found a memory leak

FTPixmapGlyph::~FTPixmapGlyph() {}

+FTGlyphImpl *FTPixmapGlyph::NewImpl(FT_GlyphSlot glyph) +{

  • FTPixmapGlyphImpl *Impl = new FTPixmapGlyphImpl(glyph);
  • if (Impl->destWidth && Impl->destHeight)
  • return Impl;
  • return new FTBitmapGlyphImpl(glyph);

There is a memory leak here (Impl).

Seems so, though not more so than in the original version which had "new" in the constructor. I tried to keep my changes minimal, so I left it as is.

It seems to affect several classes.

Using unique pointers (http://en.cppreference.com/w/cpp/memory/unique_ptr) would allow to handle that in a nice way.

Indeed, provided we may use C++11. Myself, I've been using C++14 for years now, so no problems. I don't know if Debian (CC to Manuel) or other downstream users have any objections here.

Anyway, if my code is going to be the basis for a new release (we'll see ...), I can make this change, with unique_ptr or (if someone insists) explicit "delete" in the destructor.

Regards, Frank

OlivierSohn commented 6 years ago

@frankheckenbach ,

Seems so, though not more so than in the original version which had "new" in the constructor.

I read the changes again, I confirm your change introduces a memory leak that was not there before imho. I'll provide a fix by using a delete.

EDIT : Fix is #11

OlivierSohn commented 6 years ago

FTGlyph destructor already deletes the object so there is no other change to do.

frankheckenbach commented 6 years ago

@frankheckenbach ,

Seems so, though not more so than in the original version which had "new" in the constructor.

I read the changes again, I confirm your change introduces a memory leak that was not there before imho. I'll provide a fix by using a delete.

Sorry, I misread the code. I think there's actually no memory leak (before and after my change), since the new impl is passed to the FTGlyph constructor, and its destructor deletes the object as you say. So I think your proposed change would actually introduce a double delete.

(I agree that smart pointers would be nicer and avoid all those issues, but for now I'll keep them out for backward-compatibility.)

OlivierSohn commented 6 years ago

So I think your proposed change would actually introduce a double delete

I disagree, please take another look.

frankheckenbach commented 6 years ago

+FTGlyphImpl *FTPixmapGlyph::NewImpl(FT_GlyphSlot glyph) +{

  • FTPixmapGlyphImpl *Impl = new FTPixmapGlyphImpl(glyph);
  • if (Impl->destWidth && Impl->destHeight)

Can you please add a line of comment to explain what is the intent?

It seems you want to provide a fallback, in case ... what? (please explain in more higher-level terms than width or height is zero).

The explanation is in my original bug report (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=589601) which sat in the DBTS for almost 10 years while I wasn't even aware of these versions here:

FTPixmapGlyph also doesn't check whether its impl is valid, which it would not be, again, in case of a bitmap type. In this case, it doesn't crash (in my tests), but draws garbage. The patch adds this check, and in this case just creates an FTBitmapGlyphImpl and delegates the work to it.

OlivierSohn commented 6 years ago

@frankheckenbach I see, thanks for explaining.

frankheckenbach commented 6 years ago

So I think your proposed change would actually introduce a double delete

I disagree, please take another look.

You're right, I missed this branch. I've pulled it into my repo as well.

OlivierSohn commented 6 years ago

@frankheckenbach also, the tests added by this Pull Request are referencing fonts with absolute paths, I think it would be easier to copy the font files in this repository (in a font folder) so that it's available at the same location on all systems. Else tests will fail just because the font is not found...

frankheckenbach commented 6 years ago

@frankheckenbach also, the tests added by this Pull Request are referencing fonts with absolute paths, I think it would be easier to copy the font files in this repository (in a font folder) so that it's available at the same location on all systems. Else tests will fail just because the font is not found...

OK. (Though there are more fonts missing for other test and demo programs, see test/font_pack/README.txt ... Geocities ... yeah! :)

frankheckenbach commented 6 years ago

Hi Richard,

could you please apply the 2 open pull requests for ftgl? Olivier Sohn sent his bugfix to my patch to your repository. If you pull it, I can then pull it into mine. Thanks.

Regards, Frank

manuelafm commented 5 years ago

@ulrichard @frankheckenbach Sorry for the massive delay. I've been in contact with Frank in the last few weeks though, and I'm going to update the project with these new updates, Frank will be participating in its maintenance in Debian (maybe as main maintainer :) ).

So I suppose that I should treat Frank's https://github.com/frankheckenbach/ftgl as the main upstream, or is there any objection in the end?

ulrichard commented 5 years ago

I am happy that the project found new maintainers. It is a while since I last used it. Can you make a pull request with an updated README that makes clear where the main development repo is, and who to contact. I will happily merge it. This should forward people from my repo to Frank.

manuelafm commented 5 years ago

@ulrichard @frankheckenbach I can do it if you prefer, but I think that if it's done by Frank it will be better because it's a clear and publicly documented transfer between the two people involved, specially if it's the last commit in the repo.

frankheckenbach commented 5 years ago

@ulrichard @frankheckenbach Sorry for the massive delay. I've been in contact with Frank in the last few weeks though, and I'm going to update the project with these new updates, Frank will be participating in its maintenance in Debian (maybe as main maintainer :) ).

So I suppose that I should treat Frank's https://github.com/frankheckenbach/ftgl as the main upstream, or is there any objection in the end?

No objection from me. (I suppose Salsa cannot serve as the main upstream, can it?)

Cheers, Frank

manuelafm commented 5 years ago

@frankheckenbach In principle that's not a standard practice and I don't know if there are exceptions or how to get them approved, so I think that it's better to keep it somewhere else and not on Debian's main VCS system.

frankheckenbach commented 5 years ago

@ulrichard @frankheckenbach I can do it if you prefer, but I think that if it's done by Frank it will be better because it's a clear and publicly documented transfer between the two people involved, specially if it's the last commit in the repo.

I did it now as part of the 2.3.0 patch and sent a pull request (d48ee41).