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

Better option handling, add filter iterator, expose filter options #243

Closed
wants to merge 2 commits into from

Conversation

skirsdeda
Copy link
Contributor

Options:

  • don't add AV_OPT_TYPE_CONST options to Descriptor.options, instead implement OptionChoice and Option.choices (work just like print_options in ffmpeg
  • add min, max, default_val to Option
  • from what I see, all option types are now declared in both ffmpeg and libav but this depends on which versions PyAV is targeting. Any suggestions?

Filters:

  • add FiltersIter to iterate through all registered filters using avfilter_next
  • expose descriptor and it's options in Filter

Considerations:

  • maybe it would make sense to use editorconfig because whitespace is rather messy at places

@mikeboers
Copy link
Member

Looks pretty great.

I'm not keen on FiltersIter given how the rest of the classes of objects just make a set with the available names. I was mimicking how hashlib.algorithms works. I guess I'm looking for an argument for making iterators for all of them (although I think I'd just go with a simple generator).

Dealing with which options exist can be a build-time check and defining macros for those which don't exist. That will nearly double the size of the checks. I wonder if there is a compile-time option, or if we can divine a smaller set of options to check the existence of (e.g. many of them which were added at once).

To be frank, I barely care about supporting LibAV anymore. If we have a nice solution for FFmpeg that is good enough for me.

I'll go look at editorconfig, because I am also getting tired of whitespace problems.

@skirsdeda
Copy link
Contributor Author

@mikeboers I thought about conditional compiling for option types but it would be a lot of work for no apparent benefit if you don't care about supporting LibAV. And it doesn't seem like anyone else cares much :) But what would be the lowest supported version of ffmpeg in this case?

Regarding FiltersIter, maybe it's best to have some generic generator implementation and reuse it for that with some other name. What are the other global lists with similar _next(*ptr) functions in ffmpeg?

@mikeboers
Copy link
Member

I took a quick look at the Libav source code, and the option enum layout is identical. I propose the dumb idea of just copying the latest FFmpeg enum definition into PyAV. Then it isn't a worry for version

* goes to look at older FFmpeg

Seems like FFmpeg added AV_OPT_TYPE_BOOL in 3.0, and AV_OPT_TYPE_CHANNEL_LAYOUT in 2.1. Otherwise there aren't any changes. The oldest FFmpeg that Travis will complain about is 2.5, so I think we can just look for bool at build time.

Regarding the iter, making a generic one sound like a huge pain (unless I go ahead with switching all the source code to be run through a template engine first). I vote for generators.

@mikeboers
Copy link
Member

See 749c9d9.

@skirsdeda
Copy link
Contributor Author

So a similar thing is in codec.codec called codecs_availible (note the typo). Then filter.filter can have filters_available to be more in line. Should it behave in the same way (be a set of filter names) then?

@mikeboers
Copy link
Member

I don't want to say it should. I want to say they should all be the same, but I don't really feel too strongly one way or the other to be honest. I'd welcome an argument for one or the other or both.

@mikeboers
Copy link
Member

LOL at the typo. We can change that. ;)

@skirsdeda
Copy link
Contributor Author

They should definitely be same in the way they work. Also 👍 on 749c9d9.

@mikeboers
Copy link
Member

Okay. My vote is just on the set of names then. I'll take care of it (unless you already are).

@skirsdeda
Copy link
Contributor Author

I'll do it, no problem. I also have to cleanup option types.

mikeboers added a commit that referenced this pull request Jun 21, 2017
@mikeboers
Copy link
Member

Cool, thanks.

I fixed the typo and changed the name of the formats set to match, and added a very simple test that they simply exist in f2474ae

(Hope I'm not stepping on your toes. I'll go do something else. 😉)

@skirsdeda
Copy link
Contributor Author

skirsdeda commented Jun 21, 2017

So apparently, the newest option type is UINT64 (only since ffmpeg 3.3). But I haven't found where it's used in options so we can leave it disabled for now.

@skirsdeda
Copy link
Contributor Author

@mikeboers Added Option.offset as well to be able to tell which options are aliases. That should be all for this PR (along with your changes in feature/options, of course).

mikeboers added a commit that referenced this pull request Jun 22, 2017
mikeboers added a commit that referenced this pull request Jun 22, 2017
@mikeboers
Copy link
Member

Done.

Thanks a ton!

@mikeboers mikeboers closed this Jun 22, 2017
mikeboers added a commit that referenced this pull request Jun 22, 2017
@skirsdeda
Copy link
Contributor Author

Cool! Looking forward to go deeper into PyAV once I get my basic setup working.

@mikeboers
Copy link
Member

Sweet.

At some point I want to get a better understanding of these descriptors, what classes to they apply to and when, and make sure they are being used everywhere they can be and to their fullest extent.

I don't think we currently supply get/set methods on options. That would be cool.

@mikeboers
Copy link
Member

Heads up!

I'm going to rewrite history for a moment. I'm going to roll back that merge, and change a few variable names to fit my personal idea of how to not stick to FFmpeg to be more Pythonic.

I'm a jerk. I know. ;)

mikeboers added a commit that referenced this pull request Jun 22, 2017
@mikeboers
Copy link
Member

@skirsdeda Per your suggestion at the very top of using editorconfig, I've added such a file, and I've written scripts/autolint which applies much of the .editorconfig, and also a bit of autopep8, and converts all print statements to functions.

🎉

@skirsdeda
Copy link
Contributor Author

@mikeboers terrific!

rodrhern pushed a commit to ZafeHome/PyAV that referenced this pull request Jun 14, 2018
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