IMHO that's an invalid test. It creates buffers with non-silence, sets the GAP flag on it and expects rganalysis to ignore the content and assume silence.

That's not the way how GAP buffers should be used, if the GAP flag is set elements *can* assume that they only contain silence but they're not *required* to assume that. The GAP flag must only be set on silence buffers.

Patch to remove that complete test follows

Created attachment 134465 [details] [review]
rganalysis-invalid-gap.diff

Remove the invalid GAP test.

rganalysis analyses GAP buffers since the change from the bug that broke this test. This is because the analyse feature that was added should also be run on silence and everything.

The analysis could be optimised to assume that GAP buffers are silence, however.

I agree that it's a bogus test to mark things as GAP and then push non-silence.

commit 633c9403622285e55a722db7694c2ed78c267804
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue May 12 11:16:48 2009 +0200

    rganalysis: Remove invalid unit test
    
    The test creates buffers with non-silence, sets the GAP
    flag on it and expects rganalysis to ignore the content and assume silence.
    
    That's not the way how GAP buffers should be used, if the GAP flag is set
    elements *can* assume that they only contain silence but they're not *required
    to assume that. The GAP flag must only be set on silence buffers.
    
    Fixes bug #582252.

The element was implementing GAP flag support, which Sebastian removed in a previous commit for no good reason.

The unit test is correct, tests can make use of element implementation details like this. The only way to verify that the element actually discards GAP buffers is to feed it non-silence GAP buffers, thus uncovering hidden processing that is supposed to be skipped.

(In reply to comment #5)
> The element was implementing GAP flag support, which Sebastian removed in a
> previous commit for no good reason.

I wouldn't say for no good reason. If you don't perform all the analyzing steps on GAP buffers too you won't get correct values as the analysis takes buffer content before the GAP buffers into account too but you know that ;)
Ignoring the GAP buffers doesn't change the overall result of the analysis but changes the per-window results that are posted as messages.

What could be done to still simply ignore GAP buffers, but only if no messages are posted... and limit the message property to be changed in states < PAUSED.

Does this sounds good to you? Or am I missing something? (Of course for the filters, etc one can implement "silence" versions that don't take the input into account but I don't think that makes much difference)

> The unit test is correct, tests can make use of element implementation details
> like this. The only way to verify that the element actually discards GAP
> buffers is to feed it non-silence GAP buffers, thus uncovering hidden
> processing that is supposed to be skipped.

Agreed, I should've run the unit test before committing and should've mentioned this in the commit message.

(In reply to comment #6)
[...]
> I wouldn't say for no good reason. If you don't perform all the analyzing steps
> on GAP buffers too you won't get correct values as the analysis takes buffer
> content before the GAP buffers into account too but you know that ;)
> Ignoring the GAP buffers doesn't change the overall result of the analysis but
> changes the per-window results that are posted as messages.

Do you have a specific use case in mind for the messages? Just wondering since the level element almost does the same thing already.

> What could be done to still simply ignore GAP buffers, but only if no messages
> are posted... and limit the message property to be changed in states < PAUSED.

I agree, this sounds like the right thing :)