Decref an object because cython increfed it [memory leak]

[Matiiss]

MRE:

import pygame
import pygame._sdl2 as pg_sdl2  # noqa

window = pygame.Window(minimized=True)
renderer = pg_sdl2.Renderer(window)

while not pygame.event.get(pygame.QUIT):
    renderer.to_surface()

Basically casting to object in cython increments the reference count, but pgSurface_New2 already returns an object with an incremented reference count, so have to decrement what cython has added on top of that.

[ankith26]

I'm not sure if this should be considered a Cython bug and reported? Or is this a known and documented thing?

[Matiiss]

I stumbled across similar concerns while searching for an answer, but everyone said that it's intended behavior, perchance, because something like <object>1 would create a Python int, for example (so, need to increment the reference count).
Anyway, some relevant bits I found:
https://stackoverflow.com/a/32562120/14531062
https://stackoverflow.com/a/56686757/14531062
https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html?highlight=%22except%20%20%22#python-objects-as-parameters-and-return-values
cython/cython#2589

So yeah, seems to be expected and I guess it does make sense if the usual usecase is to directly go from C structs and stuff to Python objects.

[ankith26]

So there must a memory leak, but I don't think it's due to the refcount being wrong. I just passed the return value of the function to sys.getrefcount, which returns the expected value.

[Matiiss]

Were you doing

surf = renderer.to_surface()
print(sys.getrefcount(surf))

or directly? Because that returns two different results.

[ankith26]

I was doing it directly, and I got a 2, which I thought is expected, but actually I was wrong. So yeah, this fix is correct.

Going through one of the links you posted, I see an alternate fix involves changing the declaration itself to avoid the cast, and IMO that is a neater solution

[Matiiss]

I have no clue how to implement that alternate fix which I assume you meant declaring surface to be a PyObject pointer and thus it gets passed by reference, but then I'd think you need to incref it if the surface is actually passed since it wouldn't get increfed automatically and I'm not even entirely sure how that mechanic works there, the current fix seems simple enough though, it's only 2 lines after all and quite familiar usage of the C API.

[ankith26]

What I was thinking, is replacing pgSurfaceObject *pgSurface_New2(SDL_Surface *info, int owner) with object pgSurface_New2(SDL_Surface *info, int owner) in video.pxd and then the cast could be removed easily

[Matiiss]

Alright, that also appears to have fixed the memory leak, much simpler as well 👍

[Matiiss]

Slight issue, this is so far failing on emscripten due to incompatible pointer types
https://github.com/pygame-community/pygame-ce/actions/runs/7533032001/job/20504804242?pr=2665#step:7:201

[ankith26]

Hrmmm, not sure why this is happening, -Werror is supposed to be ignored on cython files

[ankith26]

LGTM, thanks for the PR 🎉

Left a review for your consideration, but this PR is mergeable in its current state too. Maybe we could squash and merge this PR because the first commit is huge and the second does a lot of reverting

PS: please ignore my alt's reviews, I forgor to switch accounts 💀

[Starbuck5]

The web build is failing here.

src_c/_sdl2/video.c:20531:15: error: incompatible pointer types assigning to 'PyObject *' (aka 'struct _object *') from 'pgSurfaceObject *' [-Werror,-Wincompatible-pointer-types]
    __pyx_t_2 = pgSurface_New2(__pyx_v_surf, 1); if (unlikely(!__pyx_t_2)) __PYX_ERR(0, 1235, __pyx_L1_error)
              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[Matiiss]

The web build is failing here.

src_c/_sdl2/video.c:20531:15: error: incompatible pointer types assigning to 'PyObject *' (aka 'struct _object *') from 'pgSurfaceObject *' [-Werror,-Wincompatible-pointer-types]
    __pyx_t_2 = pgSurface_New2(__pyx_v_surf, 1); if (unlikely(!__pyx_t_2)) __PYX_ERR(0, 1235, __pyx_L1_error)
              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Yes, already mentioned that to Ankith in that review comment, not sure what to do though.

[Matiiss]

Reverted back to using Py_DECREF so that it passes tests. I think it's fine to leave it at this since the cython code won't be around forever anyway, so in a sense this is a temporary fix either way.
Of course open to suggestions regarding the WASM error.

[ankith26]

Okay looks like the alternate fix isn't passing WASM CI even after trying some workarounds. Let's go ahead with this PR, thanks! 👍

PS: let's squash and merge this PR

[MyreMylar]

LGTM 👍