diff mbox

[FFmpeg-devel] On in-tree external headers

Message ID 2b14914a-6e18-87f9-0483-d9f973e524b8@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson Nov. 5, 2017, 6:59 p.m. UTC
On 05/11/17 18:42, Carl Eugen Hoyos wrote:
> 2017-11-05 19:35 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>> On 05/11/17 18:28, Carl Eugen Hoyos wrote:
>>> 2017-11-05 15:24 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>>> On 30/10/17 19:51, Mark Thompson wrote:
>>>
>>>> "No external headers may be included in the ffmpeg tree."
>>>
>>> So you suggest to remove the Nvidia header?
>> If that specific policy is adopted then it would have to be.
> 
> Then I don't think this policy is useful.
> 
> Iirc, there is a second external header, I have never used
> either of those but judging from user response, both features
> are heavily used making a removal a no-go.

Ok, sure.

How about:

"No external headers may be added to the ffmpeg tree, unless they are for AviSynth or Nvidia."

(Documentation patch below.)

>> Alternative proposals are welcome.
> 
> I don't really understand:
> It was mentioned several times in the relevant thread - and
> I completely agree - that the fact that one external header
> is present is no argument to add another. (And iirc - and
> again I agree - it was also argued that pushing on this
> argument is not a good idea.)
> If you believe adding the AMD header makes sense, it may
> possibly be added, if you are against it, it will likely not be
> added.

I do not believe that adding the AMD header is the right thing to do.

But, I want to be clear what the rules are so that they can be applied consistently.  There currently appear to be no written rules, so I would like to write some so that we have a clear answer in this case and don't have this same dispute again in future (see V4L2 two months ago, where there was a pretty much identical argument with a contributor wanting to add their headers to the tree).

- Mark

Comments

Nicolas George Nov. 5, 2017, 7:01 p.m. UTC | #1
Le quintidi 15 brumaire, an CCXXVI, Mark Thompson a écrit :
> +AviSynth and Nvidia are exempt from this prohibition on external headers.

So if somebody wants to add new headers, they just have to propose a
patch to this sentence to add another exception? I will not vote for a
proposal like that.

By the way, what voting protocol are you following? It does not look
like the one I proposed some time ago.

Regards,
Timo Rothenpieler Nov. 5, 2017, 8:04 p.m. UTC | #2
Am 05.11.2017 um 19:59 schrieb Mark Thompson:
> On 05/11/17 18:42, Carl Eugen Hoyos wrote:
>> 2017-11-05 19:35 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>> On 05/11/17 18:28, Carl Eugen Hoyos wrote:
>>>> 2017-11-05 15:24 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>>>> On 30/10/17 19:51, Mark Thompson wrote:
>>>>
>>>>> "No external headers may be included in the ffmpeg tree."
>>>>
>>>> So you suggest to remove the Nvidia header?
>>> If that specific policy is adopted then it would have to be.
>>
>> Then I don't think this policy is useful.
>>
>> Iirc, there is a second external header, I have never used
>> either of those but judging from user response, both features
>> are heavily used making a removal a no-go.
> 
> Ok, sure.
> 
> How about:
> 
> "No external headers may be added to the ffmpeg tree, unless they are for AviSynth or Nvidia."

I don't think a strict "no external headers" rule makes sense or is a 
good idea at all. Specially if there are seemingly arbitrary exceptions.

If such a rule is to be added at all, it should list the conditions 
under which external headers can be added. And it should clearly be an 
exception.
Mark Thompson Nov. 5, 2017, 8:15 p.m. UTC | #3
On 05/11/17 19:01, Nicolas George wrote:
> Le quintidi 15 brumaire, an CCXXVI, Mark Thompson a écrit :
>> +AviSynth and Nvidia are exempt from this prohibition on external headers.
> 
> So if somebody wants to add new headers, they just have to propose a
> patch to this sentence to add another exception? I will not vote for a
> proposal like that.

I suppose it would mean that, yes.

> By the way, what voting protocol are you following? It does not look
> like the one I proposed some time ago.

I wasn't feeling I had reached the point where that was necessary - I am still trying to work out what people actually want.  The suggestion to have a vote soon on a specific hard-line proposal was, I suppose, a provocation to actually reply with something useful (all previous responses only addressed specific consequences rather than the proposal itself).  Given some clear proposals, it would then be possible to discuss between them and come to an agreed result.

If you feel it would be appropriate to label this as a Decision and follow your protocol, I would be happy to do so in following mails.

Thanks,

- Mark


(I'm not sure I necessarily agree with the "nothing is allowed" position, but in the absence of any other proposals at all I would back it for the clarity it offers.)
Mark Thompson Nov. 5, 2017, 8:23 p.m. UTC | #4
On 05/11/17 20:04, Timo Rothenpieler wrote:
> Am 05.11.2017 um 19:59 schrieb Mark Thompson:
>> On 05/11/17 18:42, Carl Eugen Hoyos wrote:
>>> 2017-11-05 19:35 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>>> On 05/11/17 18:28, Carl Eugen Hoyos wrote:
>>>>> 2017-11-05 15:24 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>>>>> On 30/10/17 19:51, Mark Thompson wrote:
>>>>>
>>>>>> "No external headers may be included in the ffmpeg tree."
>>>>>
>>>>> So you suggest to remove the Nvidia header?
>>>> If that specific policy is adopted then it would have to be.
>>>
>>> Then I don't think this policy is useful.
>>>
>>> Iirc, there is a second external header, I have never used
>>> either of those but judging from user response, both features
>>> are heavily used making a removal a no-go.
>>
>> Ok, sure.
>>
>> How about:
>>
>> "No external headers may be added to the ffmpeg tree, unless they are for AviSynth or Nvidia."
> 
> I don't think a strict "no external headers" rule makes sense or is a good idea at all. Specially if there are seemingly arbitrary exceptions.
> 
> If such a rule is to be added at all, it should list the conditions under which external headers can be added. And it should clearly be an exception.

Sounds good to me.  What should those conditions be?

Thanks,

- Mark
Timo Rothenpieler Nov. 5, 2017, 8:40 p.m. UTC | #5
>>> How about:
>>>
>>> "No external headers may be added to the ffmpeg tree, unless they are for AviSynth or Nvidia."
>>
>> I don't think a strict "no external headers" rule makes sense or is a good idea at all. Specially if there are seemingly arbitrary exceptions.
>>
>> If such a rule is to be added at all, it should list the conditions under which external headers can be added. And it should clearly be an exception.
> 
> Sounds good to me.  What should those conditions be?

For once, there should be a good reason to do so.

In case of nvidia the headers in this form is otherwise unobtainable, 
and it's also partially modified specifically for use in ffmpeg.
Getting the original headers is also not straight forward as you need an 
nvidia developer account, which you cannot just register for, but you 
need to apply for.

I also feel like whatever this rule would look like, it's already 
practiced that way. There isn't really a way not do decide this on a 
case by case basis. Luckily it's not something that comes up every other 
day.
If someone would submit random third party library headers to compat/ 
for no apparent reason other than comfort, it would certainly be rejected.
Michael Niedermayer Nov. 5, 2017, 9:44 p.m. UTC | #6
On Sun, Nov 05, 2017 at 06:59:42PM +0000, Mark Thompson wrote:
> On 05/11/17 18:42, Carl Eugen Hoyos wrote:
> > 2017-11-05 19:35 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> >> On 05/11/17 18:28, Carl Eugen Hoyos wrote:
> >>> 2017-11-05 15:24 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> >>>> On 30/10/17 19:51, Mark Thompson wrote:
> >>>
> >>>> "No external headers may be included in the ffmpeg tree."
> >>>
> >>> So you suggest to remove the Nvidia header?
> >> If that specific policy is adopted then it would have to be.
> > 
> > Then I don't think this policy is useful.
> > 
> > Iirc, there is a second external header, I have never used
> > either of those but judging from user response, both features
> > are heavily used making a removal a no-go.
> 
> Ok, sure.
> 
> How about:
> 

> "No external headers may be added to the ffmpeg tree, unless they are for AviSynth or Nvidia."

This would be unfair, iam against this

[...]
Jan Ekström Nov. 5, 2017, 10:12 p.m. UTC | #7
On Sun, Nov 5, 2017 at 10:40 PM, Timo Rothenpieler
<timo@rothenpieler.org> wrote:
> For once, there should be a good reason to do so.
>

Agreed.

> In case of nvidia the headers in this form is otherwise unobtainable, and
> it's also partially modified specifically for use in ffmpeg.
> Getting the original headers is also not straight forward as you need an
> nvidia developer account, which you cannot just register for, but you need
> to apply for.
>

Just my 20 cents, but this point is kind of less straightforward.
Sure, we like having more features and various vendors also like the
fact that we've made their things easily usable. But this also leads
to the in my honest opinion uncomfortable case where due to the first
vendor being an unfortunate REDACTED regarding the SDK, anything that
comes afterwards and handles itself properly seems to be getting less
nice handling if requested to behave like a normal thing in the
ecosystem. If we think about a case where the other vendor's SDK would
have been presented here first, and we would have made it available
with a dependency on its SDK, it would have been much less likely that
we would have added this latter feature with the headers there. But
this is just theoretical at this point since no proper SDK has come up
on either vendor's side. I think VA-API is currently the only thing
with a "proper SDK" (and the other Intel thing is one where someone
took the time to make it into a proper "SDK").

To be honest, I wouldn't be against removing nvidia's headers and
having them separate just to have the result be similar (even though
users would most definitely be confused at first). Avisynth/Avxsynth
is kind of a special kind of open source mess where having the header
inside makes sense (and let's not even go into Avisynth+ which doesn't
seem to want to be binary compatible with Avisynth 2.6).

> I also feel like whatever this rule would look like, it's already practiced
> that way. There isn't really a way not do decide this on a case by case
> basis. Luckily it's not something that comes up every other day.
> If someone would submit random third party library headers to compat/ for no
> apparent reason other than comfort, it would certainly be rejected.

Yes, the rule most certainly would contain something along the lines
of "...this is something that should be considered on a case-by-case
basis with a reasoning being mentioned and considered..."

Best regards,
Jan
Jorge Ramirez-Ortiz Nov. 6, 2017, 8:13 a.m. UTC | #8
On 11/05/2017 11:12 PM, Jan Ekstrom wrote:
>> I also feel like whatever this rule would look like, it's already practiced
>> that way. There isn't really a way not do decide this on a case by case
>> basis. Luckily it's not something that comes up every other day.
>> If someone would submit random third party library headers to compat/ for no
>> apparent reason other than comfort, it would certainly be rejected.
> Yes, the rule most certainly would contain something along the lines
> of "...this is something that should be considered on a case-by-case
> basis with a reasoning being mentioned and considered..."

+1.

also what about adding the justification to some sort of readme in the 
include path (compat/cude/README.tx, compat/avisynth/README.txt and so 
on) ; just to others trying to figure out when it is acceptable to 
include external headers have explicit guidelines.
diff mbox

Patch

diff --git a/doc/developer.texi b/doc/developer.texi
index a7b4f1d737..c76e0083ce 100644
--- a/doc/developer.texi
+++ b/doc/developer.texi
@@ -381,6 +381,16 @@  Never write to unallocated memory, never write over the end of arrays,
 always check values read from some untrusted source before using them
 as array index or other risky things.
 
+@subheading External headers
+Do not include external headers directly in FFmpeg.  Most packages provide
+suitable headers and linkable libraries when installed, and these should be
+used.  References to external packages in configure should use pkg-config
+if possible to find paths and options - ad-hoc build tests with hard-coded
+paths are to be avoided, but can be used if there is no other possibility.
+If an external package does not provide usable headers or libraries then
+please add directions for how to use it to the documentation.
+AviSynth and Nvidia are exempt from this prohibition on external headers.
+
 @subsection Documentation/Other
 @subheading Subscribe to the ffmpeg-cvslog mailing list.
 It is important to do this as the diffs of all commits are sent there and