[FFmpeg-devel,1/2] test: hlsenc: Don't generate test data with -re

This parameter artificially throttled the generation of the test sample
to take 5 seconds.
---
 tests/fate/hlsenc.mak | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Martin Storsjö <martin@martin.st> 于2020年8月3日周一 上午4:39写道：
>
> This parameter artificially throttled the generation of the test sample
> to take 5 seconds.
> ---
>  tests/fate/hlsenc.mak | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/fate/hlsenc.mak b/tests/fate/hlsenc.mak
> index dce1f377c7..3c767fd5d9 100644
> --- a/tests/fate/hlsenc.mak
> +++ b/tests/fate/hlsenc.mak
> @@ -88,7 +88,7 @@ fate-hls-list-size: CMD = framecrc -flags +bitexact -i $(TARGET_PATH)/tests/data
>  tests/data/hls_fmp4.m3u8: TAG = GEN
>  tests/data/hls_fmp4.m3u8: ffmpeg$(PROGSSUF)$(EXESUF) | tests/data
>         $(M)$(TARGET_EXEC) $(TARGET_PATH)/$< \
> -       -f lavfi -re -i "aevalsrc=cos(2*PI*t)*sin(2*PI*(440+4*t)*t):d=5" -map 0 -codec:a mp2fixed \
> +       -f lavfi -i "aevalsrc=cos(2*PI*t)*sin(2*PI*(440+4*t)*t):d=5" -map 0 -codec:a mp2fixed \
>         -hls_segment_type mpegts -hls_fmp4_init_filename now.mp4 -hls_list_size 0 \
>         -hls_time 1 -hls_segment_filename "$(TARGET_PATH)/tests/data/hls_fmp4_%d.m4s" \
>         $(TARGET_PATH)/tests/data/hls_fmp4.m3u8 2>/dev/null
> --
> 2.17.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

LGTM


Thanks
Steven

On Sun, Aug 02, 2020 at 11:38:51PM +0300, Martin Storsjö wrote:
> This parameter artificially throttled the generation of the test sample
> to take 5 seconds.
> ---
>  tests/fate/hlsenc.mak | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/fate/hlsenc.mak b/tests/fate/hlsenc.mak
> index dce1f377c7..3c767fd5d9 100644
> --- a/tests/fate/hlsenc.mak
> +++ b/tests/fate/hlsenc.mak
> @@ -88,7 +88,7 @@ fate-hls-list-size: CMD = framecrc -flags +bitexact -i $(TARGET_PATH)/tests/data
>  tests/data/hls_fmp4.m3u8: TAG = GEN
>  tests/data/hls_fmp4.m3u8: ffmpeg$(PROGSSUF)$(EXESUF) | tests/data
>  	$(M)$(TARGET_EXEC) $(TARGET_PATH)/$< \
> -	-f lavfi -re -i "aevalsrc=cos(2*PI*t)*sin(2*PI*(440+4*t)*t):d=5" -map 0 -codec:a mp2fixed \
> +	-f lavfi -i "aevalsrc=cos(2*PI*t)*sin(2*PI*(440+4*t)*t):d=5" -map 0 -codec:a mp2fixed \

is this removing the only test which tests the "-re" flag ?
if so that would reduce test coverage

thx

[...]

On Mon, 3 Aug 2020, Michael Niedermayer wrote:

> On Sun, Aug 02, 2020 at 11:38:51PM +0300, Martin Storsjö wrote:
>> This parameter artificially throttled the generation of the test sample
>> to take 5 seconds.
>> ---
>>  tests/fate/hlsenc.mak | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/fate/hlsenc.mak b/tests/fate/hlsenc.mak
>> index dce1f377c7..3c767fd5d9 100644
>> --- a/tests/fate/hlsenc.mak
>> +++ b/tests/fate/hlsenc.mak
>> @@ -88,7 +88,7 @@ fate-hls-list-size: CMD = framecrc -flags +bitexact -i $(TARGET_PATH)/tests/data
>>  tests/data/hls_fmp4.m3u8: TAG = GEN
>>  tests/data/hls_fmp4.m3u8: ffmpeg$(PROGSSUF)$(EXESUF) | tests/data
>>  	$(M)$(TARGET_EXEC) $(TARGET_PATH)/$< \
>> -	-f lavfi -re -i "aevalsrc=cos(2*PI*t)*sin(2*PI*(440+4*t)*t):d=5" -map 0 -codec:a mp2fixed \
>> +	-f lavfi -i "aevalsrc=cos(2*PI*t)*sin(2*PI*(440+4*t)*t):d=5" -map 0 -codec:a mp2fixed \
>
> is this removing the only test which tests the "-re" flag ?
> if so that would reduce test coverage

Well, tehcnically yes, but this test doesn't actually test whether the 
"-re" flag does what it's supposed to or anything like that - it only 
tests that adding the flag doesn't error out and doesn't alter the output. 
And it looks very much unintentional here.

// Martin

On Mon, 3 Aug 2020, Martin Storsjö wrote:

> On Mon, 3 Aug 2020, Michael Niedermayer wrote:
>
>> is this removing the only test which tests the "-re" flag ?
>> if so that would reduce test coverage
>
> Well, tehcnically yes, but this test doesn't actually test whether the "-re" 
> flag does what it's supposed to or anything like that - it only tests that 
> adding the flag doesn't error out and doesn't alter the output. And it looks 
> very much unintentional here.

Do you want to follow up the discussion here? I'd like at least some sort 
of acknowledgement back on the discussion before going forward with it in 
any direction.

// Martin

On Thu, Aug 06, 2020 at 10:08:33PM +0300, Martin Storsjö wrote:
> On Mon, 3 Aug 2020, Martin Storsjö wrote:
> 
> > On Mon, 3 Aug 2020, Michael Niedermayer wrote:
> > 
> > > is this removing the only test which tests the "-re" flag ?
> > > if so that would reduce test coverage
> > 
> > Well, tehcnically yes, but this test doesn't actually test whether the
> > "-re" flag does what it's supposed to or anything like that - it only
> > tests that adding the flag doesn't error out and doesn't alter the
> > output. 

yes


> > And it looks very much unintentional here.

maybe


> 
> Do you want to follow up the discussion here? I'd like at least some sort of
> acknowledgement back on the discussion before going forward with it in any
> direction.

sorry for the lack of reply, theres always more to reply to and do then i seem
to have time.

About it not fully testing things, that is of course not great but testing
things halfway is still better than not testing at all.

Also it may make sense given the large number of features we have to test
multiple per test.
For example with 200 features, it would take 200 individual tests to test each
in its own test, but only 10 tests if 4 of these would always test the same 
feature. this of course assumes all kinds of things, its more meant to show
that a bit of "chaos" in what each test tests could actually improve the
amount of information we could obtain from the pattern of failing tests

Thanks

[...]

On 8/7/2020 5:35 PM, Michael Niedermayer wrote:
> On Thu, Aug 06, 2020 at 10:08:33PM +0300, Martin Storsjö wrote:
>> On Mon, 3 Aug 2020, Martin Storsjö wrote:
>>
>>> On Mon, 3 Aug 2020, Michael Niedermayer wrote:
>>>
>>>> is this removing the only test which tests the "-re" flag ?
>>>> if so that would reduce test coverage
>>>
>>> Well, tehcnically yes, but this test doesn't actually test whether the
>>> "-re" flag does what it's supposed to or anything like that - it only
>>> tests that adding the flag doesn't error out and doesn't alter the
>>> output. 
> 
> yes
> 
> 
>>> And it looks very much unintentional here.
> 
> maybe
> 
> 
>>
>> Do you want to follow up the discussion here? I'd like at least some sort of
>> acknowledgement back on the discussion before going forward with it in any
>> direction.
> 
> sorry for the lack of reply, theres always more to reply to and do then i seem
> to have time.
> 
> About it not fully testing things, that is of course not great but testing
> things halfway is still better than not testing at all.
> 
> Also it may make sense given the large number of features we have to test
> multiple per test.
> For example with 200 features, it would take 200 individual tests to test each
> in its own test, but only 10 tests if 4 of these would always test the same 
> feature. this of course assumes all kinds of things, its more meant to show
> that a bit of "chaos" in what each test tests could actually improve the
> amount of information we could obtain from the pattern of failing tests
> 
> Thanks

If it's just about not removing coverage/testing of -re, it could be
added to some other test with less frames than this one, so it doesn't
take five seconds.

On Fri, Aug 07, 2020 at 10:35:58PM +0200, Michael Niedermayer wrote:
> On Thu, Aug 06, 2020 at 10:08:33PM +0300, Martin Storsjö wrote:
> > On Mon, 3 Aug 2020, Martin Storsjö wrote:
> > 
> > > On Mon, 3 Aug 2020, Michael Niedermayer wrote:
> > > 
> > > > is this removing the only test which tests the "-re" flag ?
> > > > if so that would reduce test coverage
> > > 
> > > Well, tehcnically yes, but this test doesn't actually test whether the
> > > "-re" flag does what it's supposed to or anything like that - it only
> > > tests that adding the flag doesn't error out and doesn't alter the
> > > output. 
> 
> yes
> 
> 
> > > And it looks very much unintentional here.
> 
> maybe
> 
> 
> > 
> > Do you want to follow up the discussion here? I'd like at least some sort of
> > acknowledgement back on the discussion before going forward with it in any
> > direction.
> 
> sorry for the lack of reply, theres always more to reply to and do then i seem
> to have time.
> 
> About it not fully testing things, that is of course not great but testing
> things halfway is still better than not testing at all.
> 
> Also it may make sense given the large number of features we have to test
> multiple per test.
> For example with 200 features, it would take 200 individual tests to test each
> in its own test, but only 10 tests if 4 of these would always test the same 
> feature. this of course assumes all kinds of things, its more meant to show
> that a bit of "chaos" in what each test tests could actually improve the
> amount of information we could obtain from the pattern of failing tests

To elaborate here a bit in relation to the context of "-re"

If you have 1 test that tests "-re" in addition to other things like is the
case in master. Its failure will not tell you if its "-re" or something else.
But if 2 tests which are otherwise unlrelated test "-re" in addition then
both failing will give a strong hint its "-re" without further debuging.

So if a test like the -re test is added on top of other tests it should be
to more than 1 if it doesnt have its own specialized test. That way its
easier to identify the cause.
Also obviously for the specific case of "-re" it should be added in such a
way that its not slowing any test down and doesnt cause any time dependant
test failures

So in the end for -re it probably makes sense to have a specialized test
instead as its a bit special and different from other features. But for other
features adding them to some existing tests seems a intriguing idea IMO

thx

[...]