| Message ID | 20180322020127.82915-1-josh@itanimul.li |
|---|---|
| State | New |
| Headers | show |
Josh de Kock (2018-03-22): > I have -ffunroll'd the macros for you Nicolas. That is not what I asked. The macros were just a way of making the poor API a little less poor, but the problem still remains: > +#ifdef CONFIG_AVDEVICE > + opaque = 0; > + if (muxdemuxers != SHOW_DEMUXERS) { > + while ((ofmt = av_outdev_iterate(&opaque))) { > + if ((!name || strcmp(ofmt-> name, name) < 0) && strcmp(ofmt-> name, last_name) > 0) { > + name = ofmt - > name; > + long_name = ofmt - > long_name; > + is_ofmt = 1; > } > - if (name && strcmp(ifmt->name, name) == 0) > - decode = 1; > } > } > + > + opaque = 0; > + if (muxdemuxers != SHOW_MUXERS) { > + while ((ifmt = av_indev_iterate(&opaque))) { > + if ((!name || strcmp(ifmt-> name, name) < 0) && strcmp(ifmt-> name, last_name) > 0) { > + name = ifmt - > name; > + long_name = ifmt - > long_name; > + is_ifmt = 1; > + } > + } > + } > +#endif There is a separate loop for devices, and I strongly oppose to that. Fix your API so that all (de)muxer-like components are returned in a single loop. Regards,
On 2018/03/22 10:29, Nicolas George wrote: > Josh de Kock (2018-03-22): >> I have -ffunroll'd the macros for you Nicolas. > > That is not what I asked. The macros were just a way of making the poor > API a little less poor, but the problem still remains: > >> +#ifdef CONFIG_AVDEVICE >> + opaque = 0; >> + if (muxdemuxers != SHOW_DEMUXERS) { >> + while ((ofmt = av_outdev_iterate(&opaque))) { >> + if ((!name || strcmp(ofmt-> name, name) < 0) && strcmp(ofmt-> name, last_name) > 0) { >> + name = ofmt - > name; >> + long_name = ofmt - > long_name; >> + is_ofmt = 1; >> } >> - if (name && strcmp(ifmt->name, name) == 0) >> - decode = 1; >> } >> } >> + >> + opaque = 0; >> + if (muxdemuxers != SHOW_MUXERS) { >> + while ((ifmt = av_indev_iterate(&opaque))) { >> + if ((!name || strcmp(ifmt-> name, name) < 0) && strcmp(ifmt-> name, last_name) > 0) { >> + name = ifmt - > name; >> + long_name = ifmt - > long_name; >> + is_ifmt = 1; >> + } >> + } >> + } >> +#endif > > There is a separate loop for devices, and I strongly oppose to that. Fix > your API so that all (de)muxer-like components are returned in a single > loop. > > Regards, > I strongly oppose using the same loop. Separating devices' iteration is one of the first steps to separating lavf from lavd.
Josh de Kock (2018-03-22): > I strongly oppose using the same loop. Separating devices' iteration is one > of the first steps to separating lavf from lavd. And I oppose separating lavf from lavd, was it not clear enough? I have given technical arguments in my first mail. Regards,
On 2018/03/22 11:19, Nicolas George wrote: > Josh de Kock (2018-03-22): >> I strongly oppose using the same loop. Separating devices' iteration is one >> of the first steps to separating lavf from lavd. > > And I oppose separating lavf from lavd, was it not clear enough? I have > given technical arguments in my first mail. > > Regards, > There is always the option to just merge lavf and lavd. The state of them being sort-of merged, but not really, isn't very good and adds a lot of complexity especially in inter-library dependencies (which are unneeded if lavf and lavd are either merged or actually separate).
Josh de Kock (2018-03-22): > There is always the option to just merge lavf and lavd. The state of them > being sort-of merged, but not really, isn't very good and adds a lot of > complexity especially in inter-library dependencies (which are unneeded if > lavf and lavd are either merged or actually separate). You are driving your reasoning the wrong way: you start from the limitations of your new API, and based on what it can do you intent huge changes to the project that affect user interface. It should be the opposite: first decide on the user interface and general design, and then make sure the API allow it. For user interface, I state: 1. FFmpeg should allow users to select devices the same way as (de)muxers, because it allows them to use devices in a basic way even when applications are not explicitly prepared for them, that makes extra features for free. Hence, I deduce: 2. All lavf APIs should treat devices the same way as (de)muxers. And I still think that the better option is to revert the new API and design a new new one, learning from the small mistakes of this one. Regards,
On 3/22/18, Nicolas George <george@nsup.org> wrote: > Josh de Kock (2018-03-22): >> There is always the option to just merge lavf and lavd. The state of them >> being sort-of merged, but not really, isn't very good and adds a lot of >> complexity especially in inter-library dependencies (which are unneeded >> if >> lavf and lavd are either merged or actually separate). > > You are driving your reasoning the wrong way: you start from the > limitations of your new API, and based on what it can do you intent huge > changes to the project that affect user interface. It should be the > opposite: first decide on the user interface and general design, and > then make sure the API allow it. > > For user interface, I state: > > 1. FFmpeg should allow users to select devices the same way as > (de)muxers, because it allows them to use devices in a basic way even > when applications are not explicitly prepared for them, that makes > extra features for free. > > Hence, I deduce: > > 2. All lavf APIs should treat devices the same way as (de)muxers. > > And I still think that the better option is to revert the new API and > design a new new one, learning from the small mistakes of this one. Please, for once just leave this project.
On Thu, 22 Mar 2018 12:32:29 +0100 Nicolas George <george@nsup.org> wrote: > Josh de Kock (2018-03-22): > > There is always the option to just merge lavf and lavd. The state of them > > being sort-of merged, but not really, isn't very good and adds a lot of > > complexity especially in inter-library dependencies (which are unneeded if > > lavf and lavd are either merged or actually separate). > > You are driving your reasoning the wrong way: you start from the > limitations of your new API, and based on what it can do you intent huge > changes to the project that affect user interface. It should be the > opposite: first decide on the user interface and general design, and > then make sure the API allow it. > > For user interface, I state: > > 1. FFmpeg should allow users to select devices the same way as > (de)muxers, because it allows them to use devices in a basic way even > when applications are not explicitly prepared for them, that makes > extra features for free. Devices are not muxers/demuxers and can behave significantly differently. Thus they should be accessible via a different API, and in particular avoid accidental use of device (de)muxers if only normal (de)muxers are wanted by the application.
On 2018/03/22 11:37, wm4 wrote: > On Thu, 22 Mar 2018 12:32:29 +0100 > Nicolas George <george@nsup.org> wrote: > >> Josh de Kock (2018-03-22): >>> There is always the option to just merge lavf and lavd. The state of them >>> being sort-of merged, but not really, isn't very good and adds a lot of >>> complexity especially in inter-library dependencies (which are unneeded if >>> lavf and lavd are either merged or actually separate). >> >> You are driving your reasoning the wrong way: you start from the >> limitations of your new API, and based on what it can do you intent huge >> changes to the project that affect user interface. It should be the >> opposite: first decide on the user interface and general design, and >> then make sure the API allow it. >> >> For user interface, I state: >> >> 1. FFmpeg should allow users to select devices the same way as >> (de)muxers, because it allows them to use devices in a basic way even >> when applications are not explicitly prepared for them, that makes >> extra features for free. > > Devices are not muxers/demuxers and can behave significantly > differently. Thus they should be accessible via a different API, and in > particular avoid accidental use of device (de)muxers if only normal > (de)muxers are wanted by the application. Merging lavd into lavf may be the best option here, as it allows us to change the return type of av_iterate_indev() etc to an AVDevice or another type which may represent an actual device as opposed to a purely input/output device (which is just implemented as an actual lavf format). I don't know; say this is merged and then we add an AVDevice, then we already have device iteration functions which return AVFormat so we would need different function names to accommodate the lavd change requiring yet another API change--so I am not entirely sure that the current patches (implementing option 1) are the best way to go about it.
Josh de Kock (2018-03-22): > Merging lavd into lavf may be the best option here, as it allows us to > change the return type of av_iterate_indev() etc to an AVDevice or another > type which may represent an actual device as opposed to a purely > input/output device (which is just implemented as an actual lavf format). I > don't know; say this is merged and then we add an AVDevice, then we already > have device iteration functions which return AVFormat so we would need > different function names to accommodate the lavd change requiring yet > another API change--so I am not entirely sure that the current patches > (implementing option 1) are the best way to go about it. I am sorry, but I have trouble understanding what you are trying to say. Maybe rephrase it with shorter sentences? There is no separate type for devices, they are coded as AVInputFormat and AVOutputFormat. That is fine, because they are designed to behave like (de)muxers and can be used anywhere a (de)muxer can be used. The result is a huge benefit for users. Just look at ffmpeg: it was designed for (de)muxers, but thanks to that it can do real-time capture and quick on-the-fly preview. The features are not as complete as with a real device API, but most of the time the basic features provided by lavf are largely enough to suit the needs. You want proof? Just look at the users mailing-list where questions are asked about dshow, X11 capture, Decklink cards, etc. If you were to change the lavd API to make it different from (de)muxers, all applications that right now can use devices automatically would lose that ability, to the detriment of users. Also, a general note about user interface design: specific choices should override general choices. In more specific words: applications can only assume what their users want or need, in general way. On the other hand, an user knows what he or she needs right now, in this specific case. That means that if the user wants to use a device, then the user should be, as much as possible, be allowed to use a device, and the application has no business trying to prevent that. The current API makes it more convenient to applications to not mess that up. Regards,
On 2018/03/22 12:07, Nicolas George wrote: > Josh de Kock (2018-03-22): >> Merging lavd into lavf may be the best option here, as it allows us to >> change the return type of av_iterate_indev() etc to an AVDevice or another >> type which may represent an actual device as opposed to a purely >> input/output device (which is just implemented as an actual lavf format). I >> don't know; say this is merged and then we add an AVDevice, then we already >> have device iteration functions which return AVFormat so we would need >> different function names to accommodate the lavd change requiring yet >> another API change--so I am not entirely sure that the current patches >> (implementing option 1) are the best way to go about it. > > I am sorry, but I have trouble understanding what you are trying to say. > Maybe rephrase it with shorter sentences? > move lavd avinputformats and avoutputformats into lavf delete lavd write new lavd aimed at actual devices
On Thu, Mar 22, 2018 at 01:07:18PM +0100, Nicolas George wrote: > Josh de Kock (2018-03-22): > > Merging lavd into lavf may be the best option here, as it allows us to > > change the return type of av_iterate_indev() etc to an AVDevice or another > > type which may represent an actual device as opposed to a purely > > input/output device (which is just implemented as an actual lavf format). I > > don't know; say this is merged and then we add an AVDevice, then we already > > have device iteration functions which return AVFormat so we would need > > different function names to accommodate the lavd change requiring yet > > another API change--so I am not entirely sure that the current patches > > (implementing option 1) are the best way to go about it. > > I am sorry, but I have trouble understanding what you are trying to say. > Maybe rephrase it with shorter sentences? > > There is no separate type for devices, they are coded as AVInputFormat > and AVOutputFormat. That is fine, because they are designed to behave > like (de)muxers and can be used anywhere a (de)muxer can be used. > > The result is a huge benefit for users. Just look at ffmpeg: it was > designed for (de)muxers, but thanks to that it can do real-time capture > and quick on-the-fly preview. The features are not as complete as with a > real device API, but most of the time the basic features provided by > lavf are largely enough to suit the needs. > > You want proof? Just look at the users mailing-list where questions are > asked about dshow, X11 capture, Decklink cards, etc. > > If you were to change the lavd API to make it different from (de)muxers, > all applications that right now can use devices automatically would lose > that ability, to the detriment of users. not taking a position on any of the suggestions in this thread (as i dont have the time ATM to properly think about them ...) but if lavd had a incompatible API then it would likely be possible to add a demuxer and muxer to lavf that gives access to all these devices. So they would then still be accessable through the (de)muxer interface. [...]
On Thu, Mar 22, 2018 at 02:01:27AM +0000, Josh de Kock wrote: [...] > +#ifdef CONFIG_AVDEVICE > + opaque = 0; > + if (muxdemuxers != SHOW_DEMUXERS) { > + while ((ofmt = av_outdev_iterate(&opaque))) { > + if ((!name || strcmp(ofmt-> name, name) < 0) && strcmp(ofmt-> name, last_name) > 0) { > + name = ofmt - > name; > + long_name = ofmt - > long_name; ^^^ this doesnt build [...]
On 2018/03/22 22:38, Michael Niedermayer wrote: > On Thu, Mar 22, 2018 at 02:01:27AM +0000, Josh de Kock wrote: > [...] >> +#ifdef CONFIG_AVDEVICE >> + opaque = 0; >> + if (muxdemuxers != SHOW_DEMUXERS) { >> + while ((ofmt = av_outdev_iterate(&opaque))) { >> + if ((!name || strcmp(ofmt-> name, name) < 0) && strcmp(ofmt-> name, last_name) > 0) { >> + name = ofmt - > name; > >> + long_name = ofmt - > long_name; > ^^^ > this doesnt build > Oh my bad. It's the same patch as above functionally (apart from the stray spaces somehow), but do you think this patch is better than the one with the macros? I guess it's maybe easier to read. I'll resend a patch for this if we even decide to use this set.
Josh de Kock (2018-03-22): > move lavd avinputformats and avoutputformats into lavf > > delete lavd Possibly ok in principle for me, but see below. > write new lavd aimed at actual devices There are already such libraries, we do not need another. The basic devices with a (de)muxer API are quite right to give many extra features with little extra cost. But why are we discussing this? It seems to me that the discussion went approximately like this: "Darn, the faucet I just bought to fix the leaky one does not fit the pipes. Well, I guess I will have to redo the whole plumbing to make it fit." The correct way of addressing the problem is to buy a new faucet with the correct size. And cut the losses if the first one cannot be refunded. I feel like the discussion is largely fueled by the cognitive bias known as "sunk cost fallacy": due to efforts invested in a solution, become attached emotionally to it and fail to see when it proves to cause more costs than benefits. Can we at least REALLY CONSIDER this: 1. Acknowledge that this issue about lavd, on top of Michael's early concerns about registering external components, has proven that the all-static approach, while elegant in many ways, is not practical. 2. Agree to revert the API as it is and discuss a better solution. 3. As for the better solution, I really propose to register the components (with av_register_something calls, like now) into an array rather than a linked list (like your proposal) that is stored in a user-provided structure that can exist in several instances (new), with a global instance of that structure for temporary compatibility. Note that with this proposal, your efforts are not wasted: most of the code can be reused, and they have taught us a valuable lesson on top of that. Regards,
On 2018/03/23 10:05, Nicolas George wrote: > Josh de Kock (2018-03-22): >> move lavd avinputformats and avoutputformats into lavf >> >> delete lavd > > Possibly ok in principle for me, but see below. > I personally think this will fix a lot of the weird interactions between the two libraries, and should be considered a serious option independent of the new API. >> write new lavd aimed at actual devices > > There are already such libraries, we do not need another. The basic > devices with a (de)muxer API are quite right to give many extra features > with little extra cost. After thinking about this for a while, I actually agree somewhat. > But why are we discussing this? It seems to me that the discussion went > approximately like this: > > "Darn, the faucet I just bought to fix the leaky one does not fit the > pipes. Well, I guess I will have to redo the whole plumbing to make it > fit." Sure, I guess but I still think it's more involved than just 'one faucet not fitting'. There's still an entire pipe which doesn't fit but was still installed with duct tape. > The correct way of addressing the problem is to buy a new faucet with > the correct size. And cut the losses if the first one cannot be > refunded. I feel like the discussion is largely fueled by the cognitive > bias known as "sunk cost fallacy": due to efforts invested in a > solution, become attached emotionally to it and fail to see when it > proves to cause more costs than benefits. I mean it's so far gone that I don't think it matters how long it takes anymore as long as it gets done, and gets done 'right'. This is a release blocker, and yes that's my bad but I do think that some earlier help (when I asked even before starting the new API) from others would have maybe avoided the current situation. > Can we at least REALLY CONSIDER this: > > 1. Acknowledge that this issue about lavd, on top of Michael's early > concerns about registering external components, has proven that the > all-static approach, while elegant in many ways, is not practical. > > 2. Agree to revert the API as it is and discuss a better solution. If you submit a set to revert it (my git skills suck), I won't block it, provided there will actually be discussion--from what I've seen, discussion only occurs after things happen, which isn't very helpful for larger more impactful changes. > 3. As for the better solution, I really propose to register the > components (with av_register_something calls, like now) into an array > rather than a linked list (like your proposal) that is stored in a > user-provided structure that can exist in several instances (new), > with a global instance of that structure for temporary compatibility. So the reason I liked having an iteration-style function was that it allowed an API user to get an array of all the components *but it didn't force them*. Your suggestion here is interesting, from what I understand it seems to be a thread-local, user-managed state of the loaded components in each library? Coupled with a new and proper registration API (the previous one wasn't ideal for registering external components. The downside of this approach is then you go in completely the opposite direction: the library requires even more 'bootstrap'. The reason for the new API wasn't just to use arrays internally, but to have no initialisation required. > Note that with this proposal, your efforts are not wasted: most of the > code can be reused, and they have taught us a valuable lesson on top of > that. I hope that in the future it will be less of a 'send a patch and we'll ACK/NACK', because some things really benefit from discussion.
Josh de Kock (2018-03-23): > I personally think this will fix a lot of the weird interactions between the > two libraries, and should be considered a serious option independent of the > new API. Maybe. But that is for another discussion. > Sure, I guess but I still think it's more involved than just 'one faucet not > fitting'. There's still an entire pipe which doesn't fit but was still > installed with duct tape. Possibly, but that still does not warrant redoing the whole plumbing. > I mean it's so far gone that I don't think it matters how long it takes > anymore as long as it gets done, and gets done 'right'. This is a release > blocker, This is one of the reasons I am against releases. Still, we can revert and take all the time we need. > and yes that's my bad but I do think that some earlier help (when I > asked even before starting the new API) from others would have maybe avoided > the current situation. > I hope that in the future it will be less of a 'send a patch and we'll > ACK/NACK', because some things really benefit from discussion. I completely agree that this project is awful in that matter. We should strive to change that and find better practices. Maybe post a mail tagged [METAPATCH] to discuss the principle of the changes, treat it like a patch in that it needs to be approved before being adopted. But once it is adopted, the discussion can move forward with the patches themselves, and objections about the principle can be disregarded. Note that this would not have helped much here, since nobody foresaw the complication about lavd. > If you submit a set to revert it (my git skills suck), I won't block it, > provided there will actually be discussion--from what I've seen, discussion > only occurs after things happen, which isn't very helpful for larger more > impactful changes. I will try to do just that, if time permits. But time is very short for me this week. > So the reason I liked having an iteration-style function was that it allowed > an API user to get an array of all the components *but it didn't force > them*. If an array is available, you can always look at only one cell. > Your suggestion here is interesting, from what I understand it seems to be a > thread-local, user-managed state of the loaded components in each library? > Coupled with a new and proper registration API (the previous one wasn't > ideal for registering external components. The downside of this approach is > then you go in completely the opposite direction: the library requires even > more 'bootstrap'. The reason for the new API wasn't just to use arrays > internally, but to have no initialisation required. Some of your comments show that I did not explain what I propose well enough, causing you to completely misunderstand it. Let me explain it more precisely. There is nothing thread-local about it. What I propose looks like this: AVFormatLibrary *lavf = avformat_library_alloc(); avformat_register_all(lavf); avdevice_register_all(lavf); ... avformat_open_input2(lavf, &ctx, url, NULL, &options); The benefit is that the global state becomes explicit, and not global: it can exist in several instances. Another benefit is that is provides a much cleaner and more reliable black-/white-listing solution that the one we currently have. I acknowledge your efforts to make things more static, but I think this discussion has proven it was a mistake. We cannot eradicate all needs for dynamic initialization, and a design that is meant for almost all static will make the few cases where static is not possible very hackish. You can observe just that by the fact that you needed to add an avpriv function to let lavd communicate with lavf. Each time an avpriv function appears, it shows there is something flawed in the design. Regards,
On Sun, Mar 25, 2018 at 03:32:33PM +0200, Nicolas George wrote: > Josh de Kock (2018-03-23): [...] > You can observe just that by the fact that you needed to add an avpriv > function to let lavd communicate with lavf. Each time an avpriv function > appears, it shows there is something flawed in the design. If this is true, in general, then we can improve designs by removing avpriv_* functions ... looking at what we have as avprivs, in the tree, i think some of these could be indeed usefull as public APIs. And we need to already keep them compatible as they are exported as avpriv too, so making such changes does indeed in some cases look like a good idea to me There are some avprivs we have currently though that are quite specific to format intgernals, i dont think that its always a flaw to use avpriv functions thus but i think we should evaluate each of the currently existing avpriv functions and check if the API wouldnt be usefull to user applications. And if it wouldnt be better to make it properly public thx [...]
On Sun, 25 Mar 2018 16:41:12 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sun, Mar 25, 2018 at 03:32:33PM +0200, Nicolas George wrote: > > Josh de Kock (2018-03-23): > [...] > > > You can observe just that by the fact that you needed to add an avpriv > > function to let lavd communicate with lavf. Each time an avpriv function > > appears, it shows there is something flawed in the design. Yes, libavdevice is flawed. It's a "separate" library, but uses a lot of internal libavformat functionality, against all API and ABI rules. This can't be fixed with a more fancy component registration scheme. > If this is true, in general, then we can improve designs by removing > avpriv_* functions ... Not necessarily. avpriv_ functions exist in the first place to avoid worse things. Like making things public that shouldn't be public. > looking at what we have as avprivs, in the tree, i think some of these > could be indeed usefull as public APIs. And we need to already keep them > compatible as they are exported as avpriv too, so making such changes does > indeed in some cases look like a good idea to me > > There are some avprivs we have currently though that are quite specific > to format intgernals, i dont think that its always a flaw to use avpriv > functions thus > > but i think we should evaluate each of the currently existing avpriv > functions and check if the API wouldnt be usefull to user applications. > And if it wouldnt be better to make it properly public This is 100% offtopic bikeshedding. Please don't make this discussion harder than it is.
On Fri, Mar 23, 2018 at 11:05:22AM +0100, Nicolas George wrote: > Josh de Kock (2018-03-22): > > move lavd avinputformats and avoutputformats into lavf > > > > delete lavd > > Possibly ok in principle for me, but see below. > > > write new lavd aimed at actual devices > > There are already such libraries, we do not need another. The basic > devices with a (de)muxer API are quite right to give many extra features > with little extra cost. > > But why are we discussing this? It seems to me that the discussion went > approximately like this: > > "Darn, the faucet I just bought to fix the leaky one does not fit the > pipes. Well, I guess I will have to redo the whole plumbing to make it > fit." > > The correct way of addressing the problem is to buy a new faucet with > the correct size. And cut the losses if the first one cannot be > refunded. I feel like the discussion is largely fueled by the cognitive > bias known as "sunk cost fallacy": due to efforts invested in a > solution, become attached emotionally to it and fail to see when it > proves to cause more costs than benefits. > > Can we at least REALLY CONSIDER this: > > 1. Acknowledge that this issue about lavd, on top of Michael's early > concerns about registering external components, has proven that the > all-static approach, while elegant in many ways, is not practical. > > 2. Agree to revert the API as it is and discuss a better solution. iam in favor of reverting the API, there is apparently discussion going on now here to design a different, better API and IMHO its better not to introduce a new API now if there is active work going on to change it. People would just start to switch to the current API only to have it deprecated in the release after that and having to replace it again If the new API stays then I will most likely have to submit some ugly hack to workaround the size explosion issue for static linking with the current API. And that for each lib not just avcodec. Thats to allow the ffmpeg ossfuzz code to grow and test more things quickly within the available resources. thx [...]
On 2018/03/25 16:21, Michael Niedermayer wrote: > On Fri, Mar 23, 2018 at 11:05:22AM +0100, Nicolas George wrote: >> Josh de Kock (2018-03-22): >>> move lavd avinputformats and avoutputformats into lavf >>> >>> delete lavd >> >> Possibly ok in principle for me, but see below. >> >>> write new lavd aimed at actual devices >> >> There are already such libraries, we do not need another. The basic >> devices with a (de)muxer API are quite right to give many extra features >> with little extra cost. >> >> But why are we discussing this? It seems to me that the discussion went >> approximately like this: >> >> "Darn, the faucet I just bought to fix the leaky one does not fit the >> pipes. Well, I guess I will have to redo the whole plumbing to make it >> fit." >> >> The correct way of addressing the problem is to buy a new faucet with >> the correct size. And cut the losses if the first one cannot be >> refunded. I feel like the discussion is largely fueled by the cognitive >> bias known as "sunk cost fallacy": due to efforts invested in a >> solution, become attached emotionally to it and fail to see when it >> proves to cause more costs than benefits. >> >> Can we at least REALLY CONSIDER this: >> >> 1. Acknowledge that this issue about lavd, on top of Michael's early >> concerns about registering external components, has proven that the >> all-static approach, while elegant in many ways, is not practical. >> > >> 2. Agree to revert the API as it is and discuss a better solution. > > iam in favor of reverting the API, there is apparently discussion going > on now here to design a different, better API and IMHO its better not to > introduce a new API now if there is active work going on to change it. Sure, if someone else reverts, designs, write, integrates, deals with the never-ending bikeshedding, fixes issues around lavf/lavd's broken shit, why not? But that's not going to happen, let's be real. If anyone actually cared enough, then it would be fixed already. It's been broken since version 0.5, the only reason people care now is because they're scared of change. It may not be the 'best' API, but if everything was designed the 'best' then we wouldn't have to deal with stuff like this. > People would just start to switch to the current API only to have it > deprecated in the release after that and having to replace it again > > If the new API stays then I will most likely have to submit some ugly > hack to workaround > the size explosion issue for static linking with the current API. What size explosion? > And that for each lib not just avcodec. > Thats to allow the ffmpeg ossfuzz code to grow and test more things > quickly within the available resources. I honestly would like to be an idealist here, but it's much more practical to just be real here. Nothing happens in this project unless the conversation begins with a patch (and even then, stuff barely happens). So in the words of others on the list: You forgot to attach your patch.
On Sun, Mar 25, 2018 at 04:35:12PM +0100, Josh de Kock wrote: > On 2018/03/25 16:21, Michael Niedermayer wrote: > >On Fri, Mar 23, 2018 at 11:05:22AM +0100, Nicolas George wrote: > >>Josh de Kock (2018-03-22): [...] > > >People would just start to switch to the current API only to have it > >deprecated in the release after that and having to replace it again > > > >If the new API stays then I will most likely have to submit some ugly > >hack to workaround > >the size explosion issue for static linking with the current API. > > What size explosion? the one for tools/target_dec_fuzzer It links everything instead of just one codec after the changes, that makes it substantially bigger > > >And that for each lib not just avcodec. > >Thats to allow the ffmpeg ossfuzz code to grow and test more things > >quickly within the available resources. > I honestly would like to be an idealist here, but it's much more practical > to just be real here. Nothing happens in this project unless the > conversation begins with a patch (and even then, stuff barely happens). So > in the words of others on the list: > > You forgot to attach your patch. yes, ill likely write&post that in case the API is not reverted. thx [...]
On Sun, 25 Mar 2018 17:46:50 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sun, Mar 25, 2018 at 04:35:12PM +0100, Josh de Kock wrote: > > On 2018/03/25 16:21, Michael Niedermayer wrote: > > >On Fri, Mar 23, 2018 at 11:05:22AM +0100, Nicolas George wrote: > > >>Josh de Kock (2018-03-22): > [...] > > > > >People would just start to switch to the current API only to have it > > >deprecated in the release after that and having to replace it again > > > > > >If the new API stays then I will most likely have to submit some ugly > > >hack to workaround > > >the size explosion issue for static linking with the current API. > > > > > What size explosion? > > the one for tools/target_dec_fuzzer > > It links everything instead of just one codec after the changes, that makes > it substantially bigger > Michael. Haven't we discussed this to DEATH. Seriously.
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 708a849f51..956eb7b974 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1250,19 +1250,11 @@ int show_license(void *optctx, const char *opt, const char *arg) return 0; } -static int is_device(const AVClass *avclass) -{ - if (!avclass) - return 0; - return AV_IS_INPUT_DEVICE(avclass->category) || AV_IS_OUTPUT_DEVICE(avclass->category); -} - static int show_formats_devices(void *optctx, const char *opt, const char *arg, int device_only, int muxdemuxers) { - AVInputFormat *ifmt = NULL; - AVOutputFormat *ofmt = NULL; + const AVInputFormat *ifmt = NULL; + const AVOutputFormat *ofmt = NULL; const char *last_name; - int is_dev; printf("%s\n" " D. = Demuxing supported\n" @@ -1270,46 +1262,63 @@ static int show_formats_devices(void *optctx, const char *opt, const char *arg, " --\n", device_only ? "Devices:" : "File formats:"); last_name = "000"; for (;;) { - int decode = 0; - int encode = 0; + int is_ifmt = 0; + int is_ofmt = 0; const char *name = NULL; const char *long_name = NULL; - if (muxdemuxers !=SHOW_DEMUXERS) { - while ((ofmt = av_oformat_next(ofmt))) { - is_dev = is_device(ofmt->priv_class); - if (!is_dev && device_only) - continue; - if ((!name || strcmp(ofmt->name, name) < 0) && - strcmp(ofmt->name, last_name) > 0) { - name = ofmt->name; + void *opaque = 0; + if (muxdemuxers != SHOW_DEMUXERS && !device_only) { + while ((ofmt = av_muxer_iterate(&opaque))) { + if ((!name || strcmp(ofmt->name, name) < 0) && strcmp(ofmt->name, last_name) > 0) { + name = ofmt->name; long_name = ofmt->long_name; - encode = 1; + is_ofmt = 1; } } } - if (muxdemuxers != SHOW_MUXERS) { - while ((ifmt = av_iformat_next(ifmt))) { - is_dev = is_device(ifmt->priv_class); - if (!is_dev && device_only) - continue; - if ((!name || strcmp(ifmt->name, name) < 0) && - strcmp(ifmt->name, last_name) > 0) { - name = ifmt->name; + + opaque = 0; + if (muxdemuxers != SHOW_MUXERS && !device_only) { + while ((ifmt = av_demuxer_iterate(&opaque))) { + if ((!name || strcmp(ifmt->name, name) < 0) && strcmp(ifmt->name, last_name) > 0) { + name = ifmt->name; long_name = ifmt->long_name; - encode = 0; + is_ifmt = 1; + } + } + } +#ifdef CONFIG_AVDEVICE + opaque = 0; + if (muxdemuxers != SHOW_DEMUXERS) { + while ((ofmt = av_outdev_iterate(&opaque))) { + if ((!name || strcmp(ofmt-> name, name) < 0) && strcmp(ofmt-> name, last_name) > 0) { + name = ofmt - > name; + long_name = ofmt - > long_name; + is_ofmt = 1; } - if (name && strcmp(ifmt->name, name) == 0) - decode = 1; } } + + opaque = 0; + if (muxdemuxers != SHOW_MUXERS) { + while ((ifmt = av_indev_iterate(&opaque))) { + if ((!name || strcmp(ifmt-> name, name) < 0) && strcmp(ifmt-> name, last_name) > 0) { + name = ifmt - > name; + long_name = ifmt - > long_name; + is_ifmt = 1; + } + } + } +#endif + if (!name) break; last_name = name; printf(" %s%s %-15s %s\n", - decode ? "D" : " ", - encode ? "E" : " ", + is_ifmt ? "D" : " ", + is_ofmt ? "E" : " ", name, long_name ? long_name:" "); } @@ -1439,15 +1448,45 @@ static char get_media_type_char(enum AVMediaType type) } } -static const AVCodec *next_codec_for_id(enum AVCodecID id, const AVCodec *prev, - int encoder) + +static int compare_codec(const void *a, const void *b) +{ + const AVCodec * const *ca = a; + const AVCodec * const *cb = b; + + return strcmp((*ca)->name, (*cb)->name); +} + +static unsigned get_codecs_sorted(enum AVCodecID id, const AVCodec ***rcodecs, int encoder) { - while ((prev = av_codec_next(prev))) { - if (prev->id == id && - (encoder ? av_codec_is_encoder(prev) : av_codec_is_decoder(prev))) - return prev; + const AVCodec *codec = NULL; + const AVCodec **codecs; + unsigned nb_codecs = 0, i = 0; + void *opaque = 0; + + while ((codec = av_codec_iterate(&opaque))) { + if (codec->id == id && + (encoder ? av_codec_is_encoder(codec) : av_codec_is_decoder(codec))) + nb_codecs++; } - return NULL; + + if (!(codecs = av_calloc(nb_codecs, sizeof(*codecs)))) { + av_log(NULL, AV_LOG_ERROR, "Out of memory\n"); + exit_program(1); + } + + opaque = 0; + while ((codec = av_codec_iterate(&opaque))) { + if (codec->id == id && + (encoder ? av_codec_is_encoder(codec) : av_codec_is_decoder(codec))) + codecs[i++] = codec; + } + + av_assert0(i == nb_codecs); + qsort(codecs, nb_codecs, sizeof(*codecs), compare_codec); + *rcodecs = codecs; + + return nb_codecs; } static int compare_codec_desc(const void *a, const void *b) @@ -1459,7 +1498,7 @@ static int compare_codec_desc(const void *a, const void *b) strcmp((*da)->name, (*db)->name); } -static unsigned get_codecs_sorted(const AVCodecDescriptor ***rcodecs) +static unsigned get_codec_descs_sorted(const AVCodecDescriptor ***rcodecs) { const AVCodecDescriptor *desc = NULL; const AVCodecDescriptor **codecs; @@ -1480,22 +1519,10 @@ static unsigned get_codecs_sorted(const AVCodecDescriptor ***rcodecs) return nb_codecs; } -static void print_codecs_for_id(enum AVCodecID id, int encoder) -{ - const AVCodec *codec = NULL; - - printf(" (%s: ", encoder ? "encoders" : "decoders"); - - while ((codec = next_codec_for_id(id, codec, encoder))) - printf("%s ", codec->name); - - printf(")"); -} - int show_codecs(void *optctx, const char *opt, const char *arg) { const AVCodecDescriptor **codecs; - unsigned i, nb_codecs = get_codecs_sorted(&codecs); + unsigned i, nb_codecs = get_codec_descs_sorted(&codecs); printf("Codecs:\n" " D..... = Decoding supported\n" @@ -1509,7 +1536,6 @@ int show_codecs(void *optctx, const char *opt, const char *arg) " -------\n"); for (i = 0; i < nb_codecs; i++) { const AVCodecDescriptor *desc = codecs[i]; - const AVCodec *codec = NULL; if (strstr(desc->name, "_deprecated")) continue; @@ -1525,22 +1551,22 @@ int show_codecs(void *optctx, const char *opt, const char *arg) printf(" %-20s %s", desc->name, desc->long_name ? desc->long_name : ""); - /* print decoders/encoders when there's more than one or their - * names are different from codec name */ - while ((codec = next_codec_for_id(desc->id, codec, 0))) { - if (strcmp(codec->name, desc->name)) { - print_codecs_for_id(desc->id, 0); - break; + for (int encoder = 0; encoder < 2; encoder++) { + const AVCodec **codec_codecs; + const int nb_codec_codecs = get_codecs_sorted(desc->id, &codec_codecs, encoder); + if (nb_codec_codecs == 1 && !strcmp(desc->name, codec_codecs[0]->name)) { + av_free(codec_codecs); + continue; } - } - codec = NULL; - while ((codec = next_codec_for_id(desc->id, codec, 1))) { - if (strcmp(codec->name, desc->name)) { - print_codecs_for_id(desc->id, 1); - break; + if (nb_codec_codecs) { + printf(" (%s: ", encoder ? "encoders" : "decoders"); + for (int j = 0; j < nb_codec_codecs; j++) { + printf("%s ", codec_codecs[j]->name); + } + printf(")"); } + av_free(codec_codecs); } - printf("\n"); } av_free(codecs); @@ -1550,7 +1576,7 @@ int show_codecs(void *optctx, const char *opt, const char *arg) static void print_codecs(int encoder) { const AVCodecDescriptor **codecs; - unsigned i, nb_codecs = get_codecs_sorted(&codecs); + unsigned i, nb_codecs = get_codec_descs_sorted(&codecs); printf("%s:\n" " V..... = Video\n" @@ -1566,8 +1592,11 @@ static void print_codecs(int encoder) for (i = 0; i < nb_codecs; i++) { const AVCodecDescriptor *desc = codecs[i]; const AVCodec *codec = NULL; + const AVCodec **codec_codecs; + const int nb_codec_codecs = get_codecs_sorted(desc->id, &codec_codecs, encoder); - while ((codec = next_codec_for_id(desc->id, codec, encoder))) { + for (int j = 0; j < nb_codec_codecs; j++) { + codec = codec_codecs[j]; printf(" %c", get_media_type_char(desc->type)); printf((codec->capabilities & AV_CODEC_CAP_FRAME_THREADS) ? "F" : "."); printf((codec->capabilities & AV_CODEC_CAP_SLICE_THREADS) ? "S" : "."); @@ -1581,6 +1610,7 @@ static void print_codecs(int encoder) printf("\n"); } + av_free(codec_codecs); } av_free(codecs); } @@ -1628,6 +1658,7 @@ int show_filters(void *optctx, const char *opt, const char *arg) { #if CONFIG_AVFILTER const AVFilter *filter = NULL; + void *opaque = 0; char descr[64], *descr_cur; int i, j; const AVFilterPad *pad; @@ -1640,7 +1671,7 @@ int show_filters(void *optctx, const char *opt, const char *arg) " V = Video input/output\n" " N = Dynamic number and/or type of input/output\n" " | = Source or sink filter\n"); - while ((filter = avfilter_next(filter))) { + while ((filter = av_filter_iterate(&opaque))) { descr_cur = descr; for (i = 0; i < 2; i++) { if (i) { @@ -1771,19 +1802,17 @@ static void show_help_codec(const char *name, int encoder) if (codec) print_codec(codec); else if ((desc = avcodec_descriptor_get_by_name(name))) { - int printed = 0; + const AVCodec **codec_codecs; - while ((codec = next_codec_for_id(desc->id, codec, encoder))) { - printed = 1; - print_codec(codec); - } - - if (!printed) { + if (get_codecs_sorted(desc->id, &codec_codecs, encoder)) { + print_codec(codec_codecs[0]); + } else { av_log(NULL, AV_LOG_ERROR, "Codec '%s' is known to FFmpeg, " "but no %s for it are available. FFmpeg might need to be " "recompiled with additional external libraries.\n", name, encoder ? "encoders" : "decoders"); } + av_free(codec_codecs); } else { av_log(NULL, AV_LOG_ERROR, "Codec '%s' is not recognized by FFmpeg.\n", name); @@ -2132,7 +2161,7 @@ double get_rotation(AVStream *st) } #if CONFIG_AVDEVICE -static int print_device_sources(AVInputFormat *fmt, AVDictionary *opts) +static int print_device_sources(const AVInputFormat *fmt, AVDictionary *opts) { int ret, i; AVDeviceInfoList *device_list = NULL; @@ -2147,7 +2176,7 @@ static int print_device_sources(AVInputFormat *fmt, AVDictionary *opts) goto fail; } - if ((ret = avdevice_list_input_sources(fmt, NULL, opts, &device_list)) < 0) { + if ((ret = avdevice_list_input_sources((AVInputFormat*)fmt, NULL, opts, &device_list)) < 0) { printf("Cannot list sources.\n"); goto fail; } @@ -2162,7 +2191,7 @@ static int print_device_sources(AVInputFormat *fmt, AVDictionary *opts) return ret; } -static int print_device_sinks(AVOutputFormat *fmt, AVDictionary *opts) +static int print_device_sinks(const AVOutputFormat *fmt, AVDictionary *opts) { int ret, i; AVDeviceInfoList *device_list = NULL; @@ -2177,7 +2206,7 @@ static int print_device_sinks(AVOutputFormat *fmt, AVDictionary *opts) goto fail; } - if ((ret = avdevice_list_output_sinks(fmt, NULL, opts, &device_list)) < 0) { + if ((ret = avdevice_list_output_sinks((AVOutputFormat*)fmt, NULL, opts, &device_list)) < 0) { printf("Cannot list sinks.\n"); goto fail; } @@ -2216,7 +2245,8 @@ static int show_sinks_sources_parse_arg(const char *arg, char **dev, AVDictionar int show_sources(void *optctx, const char *opt, const char *arg) { - AVInputFormat *fmt = NULL; + const AVInputFormat *fmt = NULL; + void *i = 0; char *dev = NULL; AVDictionary *opts = NULL; int ret = 0; @@ -2227,24 +2257,14 @@ int show_sources(void *optctx, const char *opt, const char *arg) if ((ret = show_sinks_sources_parse_arg(arg, &dev, &opts)) < 0) goto fail; - do { - fmt = av_input_audio_device_next(fmt); - if (fmt) { - if (!strcmp(fmt->name, "lavfi")) - continue; //it's pointless to probe lavfi - if (dev && !av_match_name(dev, fmt->name)) - continue; - print_device_sources(fmt, opts); - } - } while (fmt); - do { - fmt = av_input_video_device_next(fmt); - if (fmt) { - if (dev && !av_match_name(dev, fmt->name)) - continue; - print_device_sources(fmt, opts); - } - } while (fmt); + while ((fmt = av_indev_iterate(&i))) { + if (!strcmp(fmt->name, "lavfi")) + continue; //it's pointless to probe lavfi + if (dev && !av_match_name(dev, fmt->name)) + continue; + print_device_sources(fmt, opts); + } + fail: av_dict_free(&opts); av_free(dev); @@ -2254,7 +2274,8 @@ int show_sources(void *optctx, const char *opt, const char *arg) int show_sinks(void *optctx, const char *opt, const char *arg) { - AVOutputFormat *fmt = NULL; + const AVOutputFormat *fmt = NULL; + void *i = 0; char *dev = NULL; AVDictionary *opts = NULL; int ret = 0; @@ -2265,22 +2286,11 @@ int show_sinks(void *optctx, const char *opt, const char *arg) if ((ret = show_sinks_sources_parse_arg(arg, &dev, &opts)) < 0) goto fail; - do { - fmt = av_output_audio_device_next(fmt); - if (fmt) { - if (dev && !av_match_name(dev, fmt->name)) - continue; - print_device_sinks(fmt, opts); - } - } while (fmt); - do { - fmt = av_output_video_device_next(fmt); - if (fmt) { - if (dev && !av_match_name(dev, fmt->name)) - continue; - print_device_sinks(fmt, opts); - } - } while (fmt); + while ((fmt = av_outdev_iterate(&i))) { + if (dev && !av_match_name(dev, fmt->name)) + continue; + print_device_sinks(fmt, opts); + } fail: av_dict_free(&opts); av_free(dev);