carsales / pyheif

Python 3.6+ interface to libheif library
Apache License 2.0
166 stars 41 forks source link

Return ffi.buffer as data to avoid extra memory copying #52

Closed homm closed 2 years ago

homm commented 2 years ago

This saves 3-7% time depending on number of cores.

master:

$ python -m timeit -n5 -r10 -s "import pyheif; data = open('./full.norot.heic', 'rb').read()" "im = pyheif.read(data)"
5 loops, best of 10: 345 msec per loop

avoid-copy:

$ python -m timeit -n5 -r10 -s "import pyheif; data = open('./full.norot.heic', 'rb').read()" "im = pyheif.read(data)"
5 loops, best of 10: 323 msec per loop

Also this makes data writable buffer, which avoid one more copy in such cases:

master:

In [1]: import pyheif, numpy
In [2]: im = pyheif.read('./full.norot.heic')
In [3]: buf = numpy.frombuffer(im.data, dtype=numpy.uint8)
In [4]: buf[0:3] = b'123'
ValueError: assignment destination is read-only

In [5]: buf = buf.copy()
In [6]: buf[0:3] = b'123'  # now this works

avoid-copy:

In [1]: import pyheif, numpy
In [2]: im = pyheif.read('./full.norot.heic')
In [3]: buf = numpy.frombuffer(im.data, dtype=numpy.uint8)
In [4]: buf[0:3] = b'123'
homm commented 2 years ago

@ant32bit-carsales I don't know how you do this, but each your merge commit contain changes not from the current PR, but everything before.

Additionally, the last commit contain conflict markup

Screenshot 2021-11-16 at 10 00 22

I suggest rollback everything merged in this way and apply in the right way. As before, I also offer to transfer this repo to me for maintenance.

ant32bit-carsales commented 2 years ago

Hey Alexander,

Thanks for bringing this to my attention. Sourcetree didn’t show me any pending conflicts. That’s frustrating. All unit tests ran and docker built successfully. I’m out right now but will look at it ASAP. I must be missing a step in testing these if this can get through. So I’ll double check what I’m doing wrong.

Sorry, I’ve been spreading my focus a bit thin this last week or so. I asked about sharing the account but the response was to get more internal devs to help if I can. I know a couple of people who are into this stuff, but the road to organising this is just a little slow.

Get Outlook for iOShttps://aka.ms/o0ukef


From: Alexander Karpinsky @.> Sent: Tuesday, November 16, 2021 6:03:02 PM To: carsales/pyheif @.> Cc: Anthony Paes @.>; Mention @.> Subject: Re: [carsales/pyheif] Return ffi.buffer as data to avoid extra memory copying (PR #52)

@ant32bit-carsaleshttps://github.com/ant32bit-carsales I don't know how you do this, but each you merge commithttps://github.com/carsales/pyheif/commit/81e0ac7c40da5f1e074f676dea88fe15ed999597 contain changes not from the current PR, but everything before.

Additionally, the last commit contain conflict markup [Screenshot 2021-11-16 at 10 00 22]https://user-images.githubusercontent.com/128982/141936715-1ef89d70-3328-4dca-ada2-de94fbad3585.png

I suggest rollback everything merged in this way and apply in the right way. As before, I also offer to transfer this repo to me for maintenance.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/carsales/pyheif/pull/52#issuecomment-969934777, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKI7ZVPK6YX5ANSHOIRBIT3UMH62NANCNFSM5GEUUMWA.

homm commented 2 years ago

Please understand my frustration too. I've sent a ton of changes without any activity for a month. Today I was happy to see that some of the PR was merged, but now I see how it was merged and realize there is additional work required to fix this.

Let's be honest, the changes are bigger than the initial library. If you can't accept my changes properly and also can't permit do it by myself, please, roll back all my changes, I'm not interested in such collaboration.

ant32bit-carsales commented 2 years ago

OK, Alexander.

I can understand your frustration with me. I'm going to spend some time working out how things work before making any more commits. Thanks for all your help up to this point. It's been very much appreciated.