diff mbox

[FFmpeg-devel,PATCHv2] fate: Add an option for disabling the 2k/4k tests

Message ID 20191211083919.15394-1-martin@martin.st
State New
Headers show

Commit Message

Martin Storsjö Dec. 11, 2019, 8:39 a.m. UTC
When testing on a memory limited system, these tests consume a
significant amount of memory and can often fail if testing by running
multiple processes in parallel.
---
Adjusted to use ALLYES instead of a -yes-yes construct.

Also moved the 2k tests to the same option.
---
 configure             | 3 +++
 tests/fate/seek.mak   | 3 ++-
 tests/fate/vcodec.mak | 5 +++--
 3 files changed, 8 insertions(+), 3 deletions(-)

Comments

Carl Eugen Hoyos Dec. 11, 2019, 10:17 a.m. UTC | #1
Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö <martin@martin.st>:
>
> When testing on a memory limited system, these tests consume a
> significant amount of memory and can often fail if testing by running
> multiple processes in parallel.
> ---
> Adjusted to use ALLYES instead of a -yes-yes construct.
>
> Also moved the 2k tests to the same option.
> ---
>  configure             | 3 +++
>  tests/fate/seek.mak   | 3 ++-
>  tests/fate/vcodec.mak | 5 +++--
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index ca7137f341..922cd8d0ee 100755
> --- a/configure
> +++ b/configure
> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg itself):
>    --ignore-tests=TESTS     comma-separated list (without "fate-" prefix
>                             in the name) of tests whose result is ignored
>    --enable-linux-perf      enable Linux Performance Monitor API
> +  --disable-large-tests    disable tests that use a large amount of memory

I would have suggested to control this when running the tests, if the configure
setting makes sense, it should at least be possible to change the setting when
calling make.
Or is that possible anyway?

Thank you for the useful change, Carl Eugen
Martin Storsjö Dec. 11, 2019, 11:02 a.m. UTC | #2
On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:

> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö <martin@martin.st>:
>>
>> When testing on a memory limited system, these tests consume a
>> significant amount of memory and can often fail if testing by running
>> multiple processes in parallel.
>> ---
>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>
>> Also moved the 2k tests to the same option.
>> ---
>>  configure             | 3 +++
>>  tests/fate/seek.mak   | 3 ++-
>>  tests/fate/vcodec.mak | 5 +++--
>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/configure b/configure
>> index ca7137f341..922cd8d0ee 100755
>> --- a/configure
>> +++ b/configure
>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg itself):
>>    --ignore-tests=TESTS     comma-separated list (without "fate-" prefix
>>                             in the name) of tests whose result is ignored
>>    --enable-linux-perf      enable Linux Performance Monitor API
>> +  --disable-large-tests    disable tests that use a large amount of memory
>
> I would have suggested to control this when running the tests, if the configure
> setting makes sense, it should at least be possible to change the setting when
> calling make.
> Or is that possible anyway?

It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any var=value 
assignment on the make command line overrides any var=othervalue 
assignment within the makefiles themselves, but that doesn't seem very 
convenient.

But I'd like to have it as a configure option, to easily be able to set it 
e.g. in a fate setup.

//Martin
Martin Storsjö Dec. 12, 2019, 10:03 p.m. UTC | #3
On Wed, 11 Dec 2019, Martin Storsjö wrote:

> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>
>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
> <martin@martin.st>:
>>>
>>> When testing on a memory limited system, these tests consume a
>>> significant amount of memory and can often fail if testing by running
>>> multiple processes in parallel.
>>> ---
>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>
>>> Also moved the 2k tests to the same option.
>>> ---
>>>  configure             | 3 +++
>>>  tests/fate/seek.mak   | 3 ++-
>>>  tests/fate/vcodec.mak | 5 +++--
>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index ca7137f341..922cd8d0ee 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg 
> itself):
>>>    --ignore-tests=TESTS     comma-separated list (without "fate-" prefix
>>>                             in the name) of tests whose result is ignored
>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>> +  --disable-large-tests    disable tests that use a large amount of 
> memory
>>
>> I would have suggested to control this when running the tests, if the 
> configure
>> setting makes sense, it should at least be possible to change the setting 
> when
>> calling make.
>> Or is that possible anyway?
>
> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any var=value 
> assignment on the make command line overrides any var=othervalue 
> assignment within the makefiles themselves, but that doesn't seem very 
> convenient.
>
> But I'd like to have it as a configure option, to easily be able to set it 
> e.g. in a fate setup.

Any further opinions on this one - is it ok to go ahead with it in this 
form, or are changes requested?

// Martin
James Almer Dec. 12, 2019, 10:10 p.m. UTC | #4
On 12/12/2019 7:03 PM, Martin Storsjö wrote:
> On Wed, 11 Dec 2019, Martin Storsjö wrote:
> 
>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>
>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>> <martin@martin.st>:
>>>>
>>>> When testing on a memory limited system, these tests consume a
>>>> significant amount of memory and can often fail if testing by running
>>>> multiple processes in parallel.
>>>> ---
>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>
>>>> Also moved the 2k tests to the same option.
>>>> ---
>>>>  configure             | 3 +++
>>>>  tests/fate/seek.mak   | 3 ++-
>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/configure b/configure
>>>> index ca7137f341..922cd8d0ee 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg 
>> itself):
>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>> prefix
>>>>                             in the name) of tests whose result is
>>>> ignored
>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>> +  --disable-large-tests    disable tests that use a large amount of 
>> memory
>>>
>>> I would have suggested to control this when running the tests, if the 
>> configure
>>> setting makes sense, it should at least be possible to change the
>>> setting 
>> when
>>> calling make.
>>> Or is that possible anyway?
>>
>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>> var=value assignment on the make command line overrides any
>> var=othervalue assignment within the makefiles themselves, but that
>> doesn't seem very convenient.
>>
>> But I'd like to have it as a configure option, to easily be able to
>> set it e.g. in a fate setup.
> 
> Any further opinions on this one - is it ok to go ahead with it in this
> form, or are changes requested?

Configure option is fine if it can also be overridden from the command
line at runtime (like --samples and SAMPLES).
Martin Storsjö Dec. 13, 2019, 8:34 a.m. UTC | #5
On Thu, 12 Dec 2019, James Almer wrote:

> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>> 
>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>
>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>> <martin@martin.st>:
>>>>>
>>>>> When testing on a memory limited system, these tests consume a
>>>>> significant amount of memory and can often fail if testing by running
>>>>> multiple processes in parallel.
>>>>> ---
>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>
>>>>> Also moved the 2k tests to the same option.
>>>>> ---
>>>>>  configure             | 3 +++
>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/configure b/configure
>>>>> index ca7137f341..922cd8d0ee 100755
>>>>> --- a/configure
>>>>> +++ b/configure
>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg 
>>> itself):
>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>> prefix
>>>>>                             in the name) of tests whose result is
>>>>> ignored
>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>> +  --disable-large-tests    disable tests that use a large amount of 
>>> memory
>>>>
>>>> I would have suggested to control this when running the tests, if the 
>>> configure
>>>> setting makes sense, it should at least be possible to change the
>>>> setting 
>>> when
>>>> calling make.
>>>> Or is that possible anyway?
>>>
>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>> var=value assignment on the make command line overrides any
>>> var=othervalue assignment within the makefiles themselves, but that
>>> doesn't seem very convenient.
>>>
>>> But I'd like to have it as a configure option, to easily be able to
>>> set it e.g. in a fate setup.
>> 
>> Any further opinions on this one - is it ok to go ahead with it in this
>> form, or are changes requested?
>
> Configure option is fine if it can also be overridden from the command
> line at runtime (like --samples and SAMPLES).

Well, it can be overridden at runtime, but it's not with a very convenient 
name (the CONFIG_* variable). Is that ok?

// Martin
James Almer Dec. 13, 2019, 9:22 p.m. UTC | #6
On 12/13/2019 5:34 AM, Martin Storsjö wrote:
> On Thu, 12 Dec 2019, James Almer wrote:
> 
>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>
>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>
>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>> <martin@martin.st>:
>>>>>>
>>>>>> When testing on a memory limited system, these tests consume a
>>>>>> significant amount of memory and can often fail if testing by running
>>>>>> multiple processes in parallel.
>>>>>> ---
>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>>
>>>>>> Also moved the 2k tests to the same option.
>>>>>> ---
>>>>>>  configure             | 3 +++
>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/configure b/configure
>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>> --- a/configure
>>>>>> +++ b/configure
>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg 
>>>> itself):
>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>> prefix
>>>>>>                             in the name) of tests whose result is
>>>>>> ignored
>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>> +  --disable-large-tests    disable tests that use a large amount of 
>>>> memory
>>>>>
>>>>> I would have suggested to control this when running the tests, if the 
>>>> configure
>>>>> setting makes sense, it should at least be possible to change the
>>>>> setting 
>>>> when
>>>>> calling make.
>>>>> Or is that possible anyway?
>>>>
>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>> var=value assignment on the make command line overrides any
>>>> var=othervalue assignment within the makefiles themselves, but that
>>>> doesn't seem very convenient.
>>>>
>>>> But I'd like to have it as a configure option, to easily be able to
>>>> set it e.g. in a fate setup.
>>>
>>> Any further opinions on this one - is it ok to go ahead with it in this
>>> form, or are changes requested?
>>
>> Configure option is fine if it can also be overridden from the command
>> line at runtime (like --samples and SAMPLES).
> 
> Well, it can be overridden at runtime, but it's not with a very
> convenient name (the CONFIG_* variable). Is that ok?

Yes, it's a developer debug option. As long as the possibility is there,
it's ok.
Carl Eugen Hoyos Dec. 13, 2019, 9:48 p.m. UTC | #7
> Am 13.12.2019 um 09:34 schrieb Martin Storsjö <martin@martin.st>:
> 
>> On Thu, 12 Dec 2019, James Almer wrote:
>> 
>>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>> 
>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>> <martin@martin.st>:
>>>>>> 
>>>>>> When testing on a memory limited system, these tests consume a
>>>>>> significant amount of memory and can often fail if testing by running
>>>>>> multiple processes in parallel.
>>>>>> ---
>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>> 
>>>>>> Also moved the 2k tests to the same option.
>>>>>> ---
>>>>>>  configure             | 3 +++
>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>> 
>>>>>> diff --git a/configure b/configure
>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>> --- a/configure
>>>>>> +++ b/configure
>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg 
>>>> itself):
>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>> prefix
>>>>>>                             in the name) of tests whose result is
>>>>>> ignored
>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>> +  --disable-large-tests    disable tests that use a large amount of 
>>>> memory
>>>>> 
>>>>> I would have suggested to control this when running the tests, if the 
>>>> configure
>>>>> setting makes sense, it should at least be possible to change the
>>>>> setting 
>>>> when
>>>>> calling make.
>>>>> Or is that possible anyway?
>>>> 
>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>> var=value assignment on the make command line overrides any
>>>> var=othervalue assignment within the makefiles themselves, but that
>>>> doesn't seem very convenient.
>>>> 
>>>> But I'd like to have it as a configure option, to easily be able to
>>>> set it e.g. in a fate setup.
>>> Any further opinions on this one - is it ok to go ahead with it in this
>>> form, or are changes requested?
>> 
>> Configure option is fine if it can also be overridden from the command
>> line at runtime (like --samples and SAMPLES).
> 
> Well, it can be overridden at runtime, but it's not with a very convenient name (the CONFIG_* variable). Is that ok?

Ideally, this would be possible with:
make BIG=no fate

Carl Eugen
Martin Storsjö Dec. 13, 2019, 10:22 p.m. UTC | #8
On Fri, 13 Dec 2019, Carl Eugen Hoyos wrote:

>
>
>> Am 13.12.2019 um 09:34 schrieb Martin Storsjö <martin@martin.st>:
>> 
>>> On Thu, 12 Dec 2019, James Almer wrote:
>>> 
>>>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>>> 
>>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>>> <martin@martin.st>:
>>>>>>> 
>>>>>>> When testing on a memory limited system, these tests consume a
>>>>>>> significant amount of memory and can often fail if testing by running
>>>>>>> multiple processes in parallel.
>>>>>>> ---
>>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>>> 
>>>>>>> Also moved the 2k tests to the same option.
>>>>>>> ---
>>>>>>>  configure             | 3 +++
>>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/configure b/configure
>>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>>> --- a/configure
>>>>>>> +++ b/configure
>>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on FFmpeg 
>>>>> itself):
>>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>>> prefix
>>>>>>>                             in the name) of tests whose result is
>>>>>>> ignored
>>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>>> +  --disable-large-tests    disable tests that use a large amount of 
>>>>> memory
>>>>>> 
>>>>>> I would have suggested to control this when running the tests, if the 
>>>>> configure
>>>>>> setting makes sense, it should at least be possible to change the
>>>>>> setting 
>>>>> when
>>>>>> calling make.
>>>>>> Or is that possible anyway?
>>>>> 
>>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>>> var=value assignment on the make command line overrides any
>>>>> var=othervalue assignment within the makefiles themselves, but that
>>>>> doesn't seem very convenient.
>>>>> 
>>>>> But I'd like to have it as a configure option, to easily be able to
>>>>> set it e.g. in a fate setup.
>>>> Any further opinions on this one - is it ok to go ahead with it in this
>>>> form, or are changes requested?
>>> 
>>> Configure option is fine if it can also be overridden from the command
>>> line at runtime (like --samples and SAMPLES).
>> 
>> Well, it can be overridden at runtime, but it's not with a very convenient name (the CONFIG_* variable). Is that ok?
>
> Ideally, this would be possible with:
> make BIG=no fate

That requires a bit more extra intermediate variables.

One way of doing it would be this:

# Default, overriden by any "make BIG=no fate"
BIG=$(CONFIG_LARGE_TESTS)
...
TESTS-$(ENCDEC components)-$(BIG) += some-tests
TESTS += TESTS-yes-yes

While it earlier was requested to use $(ALLYES ...) instead of the 
-yes-yes construct.

Or to keep using ALLYES, we'd need yet another intermediate variable:

# Default, overriden by any "make BIG=no fate"
BIG=$(CONFIG_LARGE_TESTS)
# The same as BIG above, but with a CONFIG_ prefix
CONFIG_BIG=$(BIG)
...
TESTS-$(ALLYES components BIG) += some-tests
TESTS += TESTS-yes



James, what's your opinion on these two alternatives, if it should be 
configurable with a different variable name?

// Martin
James Almer Dec. 13, 2019, 10:54 p.m. UTC | #9
On 12/13/2019 7:22 PM, Martin Storsjö wrote:
> On Fri, 13 Dec 2019, Carl Eugen Hoyos wrote:
> 
>>
>>
>>> Am 13.12.2019 um 09:34 schrieb Martin Storsjö <martin@martin.st>:
>>>
>>>> On Thu, 12 Dec 2019, James Almer wrote:
>>>>
>>>>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>>>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>>>>
>>>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>>>> <martin@martin.st>:
>>>>>>>>
>>>>>>>> When testing on a memory limited system, these tests consume a
>>>>>>>> significant amount of memory and can often fail if testing by
>>>>>>>> running
>>>>>>>> multiple processes in parallel.
>>>>>>>> ---
>>>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>>>>
>>>>>>>> Also moved the 2k tests to the same option.
>>>>>>>> ---
>>>>>>>>  configure             | 3 +++
>>>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/configure b/configure
>>>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>>>> --- a/configure
>>>>>>>> +++ b/configure
>>>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on
>>>>>>>> FFmpeg 
>>>>>> itself):
>>>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>>>> prefix
>>>>>>>>                             in the name) of tests whose result is
>>>>>>>> ignored
>>>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>>>> +  --disable-large-tests    disable tests that use a large
>>>>>>>> amount of 
>>>>>> memory
>>>>>>>
>>>>>>> I would have suggested to control this when running the tests, if
>>>>>>> the 
>>>>>> configure
>>>>>>> setting makes sense, it should at least be possible to change the
>>>>>>> setting 
>>>>>> when
>>>>>>> calling make.
>>>>>>> Or is that possible anyway?
>>>>>>
>>>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>>>> var=value assignment on the make command line overrides any
>>>>>> var=othervalue assignment within the makefiles themselves, but that
>>>>>> doesn't seem very convenient.
>>>>>>
>>>>>> But I'd like to have it as a configure option, to easily be able to
>>>>>> set it e.g. in a fate setup.
>>>>> Any further opinions on this one - is it ok to go ahead with it in
>>>>> this
>>>>> form, or are changes requested?
>>>>
>>>> Configure option is fine if it can also be overridden from the command
>>>> line at runtime (like --samples and SAMPLES).
>>>
>>> Well, it can be overridden at runtime, but it's not with a very
>>> convenient name (the CONFIG_* variable). Is that ok?
>>
>> Ideally, this would be possible with:
>> make BIG=no fate
> 
> That requires a bit more extra intermediate variables.
> 
> One way of doing it would be this:
> 
> # Default, overriden by any "make BIG=no fate"
> BIG=$(CONFIG_LARGE_TESTS)
> ...
> TESTS-$(ENCDEC components)-$(BIG) += some-tests
> TESTS += TESTS-yes-yes
> 
> While it earlier was requested to use $(ALLYES ...) instead of the
> -yes-yes construct.
> 
> Or to keep using ALLYES, we'd need yet another intermediate variable:
> 
> # Default, overriden by any "make BIG=no fate"
> BIG=$(CONFIG_LARGE_TESTS)
> # The same as BIG above, but with a CONFIG_ prefix
> CONFIG_BIG=$(BIG)
> ...
> TESTS-$(ALLYES components BIG) += some-tests
> TESTS += TESTS-yes
> 
> 
> 
> James, what's your opinion on these two alternatives, if it should be
> configurable with a different variable name?

BIG is too generic and could be used for anything. LARGE_TESTS would be
better, and would get rid of the need to add a new custom CONFIG_
variable for the second example using ALLYES.
Martin Storsjö Dec. 13, 2019, 11:07 p.m. UTC | #10
On Fri, 13 Dec 2019, James Almer wrote:

> On 12/13/2019 7:22 PM, Martin Storsjö wrote:
>> On Fri, 13 Dec 2019, Carl Eugen Hoyos wrote:
>> 
>>>
>>>
>>>> Am 13.12.2019 um 09:34 schrieb Martin Storsjö <martin@martin.st>:
>>>>
>>>>> On Thu, 12 Dec 2019, James Almer wrote:
>>>>>
>>>>>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>>>>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>>>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>>>>>
>>>>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>>>>> <martin@martin.st>:
>>>>>>>>>
>>>>>>>>> When testing on a memory limited system, these tests consume a
>>>>>>>>> significant amount of memory and can often fail if testing by
>>>>>>>>> running
>>>>>>>>> multiple processes in parallel.
>>>>>>>>> ---
>>>>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>>>>>
>>>>>>>>> Also moved the 2k tests to the same option.
>>>>>>>>> ---
>>>>>>>>>  configure             | 3 +++
>>>>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/configure b/configure
>>>>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>>>>> --- a/configure
>>>>>>>>> +++ b/configure
>>>>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on
>>>>>>>>> FFmpeg 
>>>>>>> itself):
>>>>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>>>>> prefix
>>>>>>>>>                             in the name) of tests whose result is
>>>>>>>>> ignored
>>>>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>>>>> +  --disable-large-tests    disable tests that use a large
>>>>>>>>> amount of 
>>>>>>> memory
>>>>>>>>
>>>>>>>> I would have suggested to control this when running the tests, if
>>>>>>>> the 
>>>>>>> configure
>>>>>>>> setting makes sense, it should at least be possible to change the
>>>>>>>> setting 
>>>>>>> when
>>>>>>>> calling make.
>>>>>>>> Or is that possible anyway?
>>>>>>>
>>>>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>>>>> var=value assignment on the make command line overrides any
>>>>>>> var=othervalue assignment within the makefiles themselves, but that
>>>>>>> doesn't seem very convenient.
>>>>>>>
>>>>>>> But I'd like to have it as a configure option, to easily be able to
>>>>>>> set it e.g. in a fate setup.
>>>>>> Any further opinions on this one - is it ok to go ahead with it in
>>>>>> this
>>>>>> form, or are changes requested?
>>>>>
>>>>> Configure option is fine if it can also be overridden from the command
>>>>> line at runtime (like --samples and SAMPLES).
>>>>
>>>> Well, it can be overridden at runtime, but it's not with a very
>>>> convenient name (the CONFIG_* variable). Is that ok?
>>>
>>> Ideally, this would be possible with:
>>> make BIG=no fate
>> 
>> That requires a bit more extra intermediate variables.
>> 
>> One way of doing it would be this:
>> 
>> # Default, overriden by any "make BIG=no fate"
>> BIG=$(CONFIG_LARGE_TESTS)
>> ...
>> TESTS-$(ENCDEC components)-$(BIG) += some-tests
>> TESTS += TESTS-yes-yes
>> 
>> While it earlier was requested to use $(ALLYES ...) instead of the
>> -yes-yes construct.
>> 
>> Or to keep using ALLYES, we'd need yet another intermediate variable:
>> 
>> # Default, overriden by any "make BIG=no fate"
>> BIG=$(CONFIG_LARGE_TESTS)
>> # The same as BIG above, but with a CONFIG_ prefix
>> CONFIG_BIG=$(BIG)
>> ...
>> TESTS-$(ALLYES components BIG) += some-tests
>> TESTS += TESTS-yes
>> 
>> 
>> 
>> James, what's your opinion on these two alternatives, if it should be
>> configurable with a different variable name?
>
> BIG is too generic and could be used for anything. LARGE_TESTS would be
> better, and would get rid of the need to add a new custom CONFIG_
> variable for the second example using ALLYES.

I intentionally meant to use a different variable for that, to 
differentiate between the configure-generated CONFIG_LARGE_TESTS from 
config.mak and the one that is set to pick up a potential user-supplied 
value from e.g. LARGE_TESTS, otherwise falling back on the configure 
generated value - I'm not sure if that really works if the first and last 
variable are the same, or if that ends up as an infinite recursion 
otherwise (CONFIG_LARGE_TESTS expands to LARGE_TESTS which expands to 
CONFIG_LARGE_TESTS).

// Martin
James Almer Dec. 13, 2019, 11:16 p.m. UTC | #11
On 12/13/2019 8:07 PM, Martin Storsjö wrote:
> On Fri, 13 Dec 2019, James Almer wrote:
> 
>> On 12/13/2019 7:22 PM, Martin Storsjö wrote:
>>> On Fri, 13 Dec 2019, Carl Eugen Hoyos wrote:
>>>
>>>>
>>>>
>>>>> Am 13.12.2019 um 09:34 schrieb Martin Storsjö <martin@martin.st>:
>>>>>
>>>>>> On Thu, 12 Dec 2019, James Almer wrote:
>>>>>>
>>>>>>> On 12/12/2019 7:03 PM, Martin Storsjö wrote:
>>>>>>>> On Wed, 11 Dec 2019, Martin Storsjö wrote:
>>>>>>>>> On Wed, 11 Dec 2019, Carl Eugen Hoyos wrote:
>>>>>>>>>
>>>>>>>>> Am Mi., 11. Dez. 2019 um 09:39 Uhr schrieb Martin Storsjö 
>>>>>>>> <martin@martin.st>:
>>>>>>>>>>
>>>>>>>>>> When testing on a memory limited system, these tests consume a
>>>>>>>>>> significant amount of memory and can often fail if testing by
>>>>>>>>>> running
>>>>>>>>>> multiple processes in parallel.
>>>>>>>>>> ---
>>>>>>>>>> Adjusted to use ALLYES instead of a -yes-yes construct.
>>>>>>>>>>
>>>>>>>>>> Also moved the 2k tests to the same option.
>>>>>>>>>> ---
>>>>>>>>>>  configure             | 3 +++
>>>>>>>>>>  tests/fate/seek.mak   | 3 ++-
>>>>>>>>>>  tests/fate/vcodec.mak | 5 +++--
>>>>>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/configure b/configure
>>>>>>>>>> index ca7137f341..922cd8d0ee 100755
>>>>>>>>>> --- a/configure
>>>>>>>>>> +++ b/configure
>>>>>>>>>> @@ -482,6 +482,7 @@ Developer options (useful when working on
>>>>>>>>>> FFmpeg 
>>>>>>>> itself):
>>>>>>>>>>    --ignore-tests=TESTS     comma-separated list (without "fate-"
>>>>>>>>>> prefix
>>>>>>>>>>                             in the name) of tests whose result is
>>>>>>>>>> ignored
>>>>>>>>>>    --enable-linux-perf      enable Linux Performance Monitor API
>>>>>>>>>> +  --disable-large-tests    disable tests that use a large
>>>>>>>>>> amount of 
>>>>>>>> memory
>>>>>>>>>
>>>>>>>>> I would have suggested to control this when running the tests, if
>>>>>>>>> the 
>>>>>>>> configure
>>>>>>>>> setting makes sense, it should at least be possible to change the
>>>>>>>>> setting 
>>>>>>>> when
>>>>>>>>> calling make.
>>>>>>>>> Or is that possible anyway?
>>>>>>>>
>>>>>>>> It's possible to do e.g. "make fate CONFIG_LARGE_TESTS=no"; any
>>>>>>>> var=value assignment on the make command line overrides any
>>>>>>>> var=othervalue assignment within the makefiles themselves, but that
>>>>>>>> doesn't seem very convenient.
>>>>>>>>
>>>>>>>> But I'd like to have it as a configure option, to easily be able to
>>>>>>>> set it e.g. in a fate setup.
>>>>>>> Any further opinions on this one - is it ok to go ahead with it in
>>>>>>> this
>>>>>>> form, or are changes requested?
>>>>>>
>>>>>> Configure option is fine if it can also be overridden from the
>>>>>> command
>>>>>> line at runtime (like --samples and SAMPLES).
>>>>>
>>>>> Well, it can be overridden at runtime, but it's not with a very
>>>>> convenient name (the CONFIG_* variable). Is that ok?
>>>>
>>>> Ideally, this would be possible with:
>>>> make BIG=no fate
>>>
>>> That requires a bit more extra intermediate variables.
>>>
>>> One way of doing it would be this:
>>>
>>> # Default, overriden by any "make BIG=no fate"
>>> BIG=$(CONFIG_LARGE_TESTS)
>>> ...
>>> TESTS-$(ENCDEC components)-$(BIG) += some-tests
>>> TESTS += TESTS-yes-yes
>>>
>>> While it earlier was requested to use $(ALLYES ...) instead of the
>>> -yes-yes construct.
>>>
>>> Or to keep using ALLYES, we'd need yet another intermediate variable:
>>>
>>> # Default, overriden by any "make BIG=no fate"
>>> BIG=$(CONFIG_LARGE_TESTS)
>>> # The same as BIG above, but with a CONFIG_ prefix
>>> CONFIG_BIG=$(BIG)
>>> ...
>>> TESTS-$(ALLYES components BIG) += some-tests
>>> TESTS += TESTS-yes
>>>
>>>
>>>
>>> James, what's your opinion on these two alternatives, if it should be
>>> configurable with a different variable name?
>>
>> BIG is too generic and could be used for anything. LARGE_TESTS would be
>> better, and would get rid of the need to add a new custom CONFIG_
>> variable for the second example using ALLYES.
> 
> I intentionally meant to use a different variable for that, to
> differentiate between the configure-generated CONFIG_LARGE_TESTS from
> config.mak and the one that is set to pick up a potential user-supplied
> value from e.g. LARGE_TESTS, otherwise falling back on the configure
> generated value - I'm not sure if that really works if the first and
> last variable are the same, or if that ends up as an infinite recursion
> otherwise (CONFIG_LARGE_TESTS expands to LARGE_TESTS which expands to
> CONFIG_LARGE_TESTS).

I still think just overriding using CONFIG_LARGE_TESTS from the command
line is more than enough for a developer debug option, but if others
want something shorter then your suggestion above for ALLYES is fine.
Carl Eugen Hoyos Dec. 14, 2019, 4:36 p.m. UTC | #12
Am Sa., 14. Dez. 2019 um 00:02 Uhr schrieb James Almer <jamrial@gmail.com>:

> BIG is too generic

Within fate?

Carl Eugen
James Almer Dec. 14, 2019, 4:41 p.m. UTC | #13
On 12/14/2019 1:36 PM, Carl Eugen Hoyos wrote:
> Am Sa., 14. Dez. 2019 um 00:02 Uhr schrieb James Almer <jamrial@gmail.com>:
> 
>> BIG is too generic
> 
> Within fate?
> 
> Carl Eugen

For a make option. It could be anything. Big tests? Big binaries?

I already said i'm fine with it if no other option is ok for other devs,
so we can move on.
Martin Storsjö Dec. 14, 2019, 11:12 p.m. UTC | #14
On Fri, 13 Dec 2019, James Almer wrote:

> On 12/13/2019 8:07 PM, Martin Storsjö wrote:
>> I intentionally meant to use a different variable for that, to
>> differentiate between the configure-generated CONFIG_LARGE_TESTS from
>> config.mak and the one that is set to pick up a potential user-supplied
>> value from e.g. LARGE_TESTS, otherwise falling back on the configure
>> generated value - I'm not sure if that really works if the first and
>> last variable are the same, or if that ends up as an infinite recursion
>> otherwise (CONFIG_LARGE_TESTS expands to LARGE_TESTS which expands to
>> CONFIG_LARGE_TESTS).
>
> I still think just overriding using CONFIG_LARGE_TESTS from the command
> line is more than enough for a developer debug option, but if others
> want something shorter then your suggestion above for ALLYES is fine.

FWIW, I think it can be handled pretty neatly in two different ways, at 
the bottom of config.mak:

CONFIG_LARGE_TESTS:=$(if $(LARGE_TESTS), $(LARGE_TESTS), $(CONFIG_LARGE_TESTS))

This also is picked up if you'd have "export LARGE_TESTS=no; ... other 
stuff ... ; make fate". Picking up random unprefixed variables like this 
might not be ideal.

Alternatively, slightly weirder looking but guarding against that, would 
be:

# If LARGE_TESTS=foo is set on the make command line, this is ignored
LARGE_TESTS:=$(CONFIG_LARGE_TESTS)
# Overwrite CONFIG_LARGE_TESTS with whatever LARGE_TESTS was set to.
CONFIG_LARGE_TESTS:=$(LARGE_TESTS)

// Martin
diff mbox

Patch

diff --git a/configure b/configure
index ca7137f341..922cd8d0ee 100755
--- a/configure
+++ b/configure
@@ -482,6 +482,7 @@  Developer options (useful when working on FFmpeg itself):
   --ignore-tests=TESTS     comma-separated list (without "fate-" prefix
                            in the name) of tests whose result is ignored
   --enable-linux-perf      enable Linux Performance Monitor API
+  --disable-large-tests    disable tests that use a large amount of memory
 
 NOTE: Object files are built at the place where configure is launched.
 EOF
@@ -1931,6 +1932,7 @@  CONFIG_LIST="
     $SUBSYSTEM_LIST
     autodetect
     fontconfig
+    large_tests
     linux_perf
     memory_poisoning
     neon_clobber_test
@@ -3724,6 +3726,7 @@  enable asm
 enable debug
 enable doc
 enable faan faandct faanidct
+enable large_tests
 enable optimizations
 enable runtime_cpudetect
 enable safe_bitstream_reader
diff --git a/tests/fate/seek.mak b/tests/fate/seek.mak
index 04f54ee6c4..98d2b54674 100644
--- a/tests/fate/seek.mak
+++ b/tests/fate/seek.mak
@@ -64,7 +64,6 @@  FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, ASV1,          AVI)     += asv1
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, ASV2,          AVI)     += asv2
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, DNXHD,         DNXHD)   += dnxhd-720p
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, DNXHD,         DNXHD)   += dnxhd-720p-rd
-FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, DNXHD,         DNXHD)   += dnxhd-4k-hr-lb
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, DNXHD,         MOV)     += dnxhd-1080i
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, DVVIDEO,       DV)      += dv
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, DVVIDEO,       DV)      += dv-411
@@ -80,6 +79,8 @@  FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, JPEGLS,        AVI)     += jpegls
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, LJPEG MJPEG,   AVI)     += ljpeg
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, MJPEG,         AVI)     += mjpeg
 
+FATE_SEEK_VSYNTH_LENA-$(call ALLYES, DNXHD_ENCODER DNXHD_DECODER LARGE_TESTS) += dnxhd-4k-hr-lb
+
 FATE_SEEK_VSYNTH_LENA-$(call ENCDEC, MPEG1VIDEO, MPEG1VIDEO MPEGVIDEO) +=    \
                                                     mpeg1                    \
                                                     mpeg1b
diff --git a/tests/fate/vcodec.mak b/tests/fate/vcodec.mak
index 452246689e..fc27da5456 100644
--- a/tests/fate/vcodec.mak
+++ b/tests/fate/vcodec.mak
@@ -29,13 +29,14 @@  FATE_VCODEC-$(call ENCDEC, DNXHD, DNXHD) += dnxhd-720p                  \
                                             dnxhd-720p-rd               \
                                             dnxhd-720p-10bit            \
                                             dnxhd-720p-hr-lb            \
-                                            dnxhd-4k-hr-lb              \
                                             dnxhd-uhd-hr-sq             \
-                                            dnxhd-2k-hr-hq              \
                                             dnxhd-edge1-hr              \
                                             dnxhd-edge2-hr              \
                                             dnxhd-edge3-hr
 
+FATE_VCODEC-$(call ALLYES, DNXHD_ENCODER DNXHD_DECODER LARGE_TESTS) += dnxhd-4k-hr-lb \
+                                                                       dnxhd-2k-hr-hq
+
 FATE_VCODEC-$(call ENCDEC, VC2 DIRAC, MOV) += vc2-420p vc2-420p10 vc2-420p12 \
                                               vc2-422p vc2-422p10 vc2-422p12 \
                                               vc2-444p vc2-444p10 vc2-444p12 \