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

Fix some use-after-frees and memory leak in the wmf decoder #962

Merged
merged 2 commits into from Oct 11, 2023

Conversation

Square789
Copy link
Contributor

This says "some" cause the video part is intentionally using a use-after-free to expose the frame.
In any case, the primary reason for this PR is to fix the jumping leak from #926 stemming from the _source_reader not being released, now that the other one turned out to be a Python bug.

@caffeinepills
Copy link
Collaborator

caffeinepills commented Oct 4, 2023

This causes audio to skip and crackle on my machine. The reason buffers and samples were not a local variable was because the data can get GC'd during playback in some drivers if not stored somewhere, and has to be freed up after it's been played, not before.

@Square789
Copy link
Contributor Author

Square789 commented Oct 4, 2023

Thanks for the quick report, caught me by surprise; it works fine for me with both DirectSound and XAudio2.
Even before, the audio data was copied via memmove, there's no reasonable way the playback mechanism could still have any kind of access to the IMFBuffer/Samples. Maybe i'm releasing the imf_sample too early, the MS docs don't mention anything explicitly, but maybe it deletes its buffers even if they still have references.

@caffeinepills
Copy link
Collaborator

I updated to the latest with sample being freed last, and it seems to have fixed the crackling for me. (Windows 11, Xaudio2, ogg/mp3 testing)

@caffeinepills
Copy link
Collaborator

Also, the video portion of WMF can probably just be removed in the future as it shouldn't be used anymore.

@benmoran56
Copy link
Member

Shall we merge this for now, and then strip out the video parts in the development branch?

@Square789
Copy link
Contributor Author

I don't really know what the status with the wmf decoder's video section is. I could not get it to play any video in the first place. The _debug at

assert _debug('WMFVideoDecoder: Found Video Stream')
is triggered, but then this bit:
try:
self._source_reader.SetCurrentMediaType(self._video_stream_index, None, uncompressed_mt)
except OSError as err: # Can't decode codec.
raise DecodeException(err) from None
always fails and reports [WinError -1072875852] The data specified for the media type is invalid, inconsistent, or not supported by this object for a variety of videos in different formats i've thrown at it.
I was focusing this PR on fixing issues in the audio section only; i feel any changes regarding the video part are kinda out of scope for this PR

@pushfoo
Copy link
Contributor

pushfoo commented Oct 11, 2023

I'm still hesitant about tearing out the WMF video decoding. If we can get MP4 to work and establish a baseline for the video stream across Mac & Windows, we'd have a 0-pyglet video equivalent to MP3. That could be a significant decrease in download size.

@caffeinepills
Copy link
Collaborator

always fails and reports [WinError -1072875852] The data specified for the media type is invalid, inconsistent, or not supported by this object for a variety of videos in different formats i've thrown at it.
I was focusing this PR on fixing issues in the audio section only; i feel any changes regarding the video part are kinda out of scope for this PR

The formats are somewhat limited I believe. Video stuff can be removed at a later date.

I'm still hesitant about tearing out the WMF video decoding. If we can get MP4 to work and establish a baseline for the video stream across Mac & Windows, we'd have a 0-pyglet video equivalent to MP3. That could be a significant decrease in download size.

Unfortunately with WMF it's not hardware accelerated without DirectX and I am not quite up to the task of creating a DirectX-OpenGL link just to make video work for whatever formats it supports when FFmpeg does it all better.

We can leave the video stuff for another PR later down the road.

@caffeinepills caffeinepills merged commit 4c41583 into pyglet:master Oct 11, 2023
11 checks passed
@Square789 Square789 deleted the wmf_fix branch December 21, 2023 06:26
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

4 participants