Skip to content

Conversation

@Kazhuu
Copy link

@Kazhuu Kazhuu commented Sep 26, 2021

This PR relates to this #702 issue I posted earlier. This let user to specify input audio codec to be used instead of relaying on FFMPEG default one. Default codec is PCM_S16LE and in one scenario I needed to specify PCM_S32LE instead.

Some questions still on my head related to this:

  • I only added PCM codecs. Is this okay? I think in the future if someone needs more they can be added quite easily.
  • Anything to improve here? I'd be happy to modify this PR if needed.

With this change av.open can for example be called with

import av
av.open('alsa_device_name', format='alsa', audio_codec='PCM_S32LE')

@jlaine
Copy link
Member

jlaine commented Mar 27, 2022

Could we have some unit tests for this please?

Also the docstring should clarify what the usecase for this is: AFAICT this only makes sense for reading from audio devices (ALSA + pulse).

@Kazhuu Kazhuu force-pushed the user_specified_audio_codec branch from 29d3a98 to bcd9c00 Compare August 12, 2022 12:31
@Kazhuu Kazhuu force-pushed the user_specified_audio_codec branch 2 times, most recently from 77923f7 to abfb5f1 Compare August 15, 2022 08:42
@Kazhuu Kazhuu force-pushed the user_specified_audio_codec branch from abfb5f1 to 3a3f061 Compare August 15, 2022 08:43
@Kazhuu Kazhuu requested a review from jlaine August 15, 2022 10:47
@Kazhuu
Copy link
Author

Kazhuu commented Aug 15, 2022

@jlaine do you think it would make sense to add documentation or something to list the supported codecs? I added a simple test but was thinking could I somehow test the default codec somehow when the audio_codec is not given. I couldn't find tests that open any alsa devices.

@WyattBlue
Copy link
Member

I'm closing this because this PR needs to be redone. It has merge conflicts and uses an old code style, (we prefer double quotes now).

I don't know if jlaine would agree with me but I think av.open("alsa_device_name", format="alsa:pcm_s32le") would be a better interface, (the parsing would be implemented in PyAV). I would think along these lines if you were to redo this.

For me, this is a very low priority feature. It's probably a low priority for jlaine too. I don't know anyone else who needs this. I wouldn't it if it were to be implemented, but it would need to be done in another PR.

@WyattBlue WyattBlue closed this Feb 2, 2024
@Kazhuu
Copy link
Author

Kazhuu commented Feb 2, 2024

Hello @WyattBlue. Thanks for commenting on this one. I don't really need this feature anymore so I don't see myself working on this PR again, also because the process has been quite slow I always forgot what I did last time. So this can be closed and the issue I opened also be closed. Maybe someone else who also needs this can find this and continue the work. Thanks for you time!

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.

Allow user to force audio codec to be used instead of relying the default one

3 participants