Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MuPDF: fix possible memory leaks #592

Merged
merged 2 commits into from
Jan 30, 2018
Merged

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Jan 30, 2018

Not really sure the fz_drop_colorspace() are really needed (or they are small datastructures that we don't see growing), but it seems logical, and they don't seem to hurt.
But the FIXME: undefined pixmap, whick feels like a typo, looks like there were pix there never freed - but if that was the case, we should have seen big memory leaks (at least people who read pdf, which I rarely do).

I've been trying to understand and hunt possible memory leaks with the html dictionary, where I see a huge grow in mem use when used - but may be it's not unlimited. On the emulator, with mupdf.lua debug_memory = true, it seems it grows quickly to +30Mb, but then try to stays at around 30/35.
I didn't find any such 30/32/35/256<<17 in MuPDF sources, so I don't know where that is made out and ensured...
On my Kobo, memory seems to grow quickly from 30Mb to 70/100MB, and then may grow less quickly,
So, it may be the same thing as observed on the emulaor - lua objects stacked up at the end of memory, make it grows slowly, and the big mupdf cache(?) is not released, which should be ok.

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@poire-z
Copy link
Contributor Author

poire-z commented Jan 30, 2018

Mhhh, looking again, I'm not so sure about the fz_drop_colorspace() .
Seem logical from a documentation/include point of view, but the fz_device_gray/rgb we call just return references to stuff in the context, without any keep/drop reference counting.
https://github.com/ArtifexSoftware/mupdf/blob/148329c60f8ad07886b624b5e9e3d8dfdbbdb98f/source/fitz/colorspace.c#L902-L912

edit: I'm going to assume that if there is no fz_new in fz_device_gray, we don't need to fz_drop it :)
edit2: some comment in ArtifexSoftware/mupdf@321ba1d says fz_device_* only borrows reference , so we were fine with no fz_drop_colorspace.

@Frenzie Frenzie merged commit 7841fc1 into koreader:master Jan 30, 2018
@poire-z poire-z deleted the mupdf_free branch January 30, 2018 15:44
Frenzie added a commit to Frenzie/koreader that referenced this pull request Jan 30, 2018
Includes:

* MuPDF: fix possible memory leaks <koreader/koreader-base#592>
* Bump luasocket <koreader/koreader-base#593>; fixes koreader#3596
Frenzie added a commit to koreader/koreader that referenced this pull request Jan 30, 2018
Includes:

* MuPDF: fix possible memory leaks <koreader/koreader-base#592>
* Bump luasocket <koreader/koreader-base#593>; fixes #3596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants