diff mbox

[FFmpeg-devel,v4,3/7] cmdutils: use new iteration APIs

Message ID 20180322020127.82915-1-josh@itanimul.li
State New
Headers show

Commit Message

Josh de Kock March 22, 2018, 2:01 a.m. UTC
---

 I have -ffunroll'd the macros for you Nicolas.

 fftools/cmdutils.c | 248 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 129 insertions(+), 119 deletions(-)

Comments

Nicolas George March 22, 2018, 10:29 a.m. UTC | #1
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,
Josh de Kock March 22, 2018, 11:19 a.m. UTC | #2
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.
Nicolas George March 22, 2018, 11:19 a.m. UTC | #3
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,
Josh de Kock March 22, 2018, 11:24 a.m. UTC | #4
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).
Nicolas George March 22, 2018, 11:32 a.m. UTC | #5
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,
Paul B Mahol March 22, 2018, 11:33 a.m. UTC | #6
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.
wm4 March 22, 2018, 11:37 a.m. UTC | #7
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.
Josh de Kock March 22, 2018, 11:48 a.m. UTC | #8
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.
Nicolas George March 22, 2018, 12:07 p.m. UTC | #9
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,
Josh de Kock March 22, 2018, 12:12 p.m. UTC | #10
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
Michael Niedermayer March 22, 2018, 7:42 p.m. UTC | #11
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.


[...]
Michael Niedermayer March 22, 2018, 10:38 p.m. UTC | #12
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

[...]
Josh de Kock March 23, 2018, 12:06 a.m. UTC | #13
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.
Nicolas George March 23, 2018, 10:05 a.m. UTC | #14
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,
Josh de Kock March 23, 2018, 10:56 a.m. UTC | #15
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.
Nicolas George March 25, 2018, 1:32 p.m. UTC | #16
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,
Michael Niedermayer March 25, 2018, 2:41 p.m. UTC | #17
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

[...]
wm4 March 25, 2018, 2:48 p.m. UTC | #18
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.
Michael Niedermayer March 25, 2018, 3:21 p.m. UTC | #19
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


[...]
Josh de Kock March 25, 2018, 3:35 p.m. UTC | #20
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.
Michael Niedermayer March 25, 2018, 3:46 p.m. UTC | #21
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

[...]
wm4 March 25, 2018, 3:57 p.m. UTC | #22
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 mbox

Patch

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);