[FFmpeg-devel] [PATCH v2 1/2] avcodec/noise: don't force non-zero value for amount

[Gyan Doshi]

On 2021-07-24 03:19, Michael Niedermayer wrote:
> On Sat, Jul 24, 2021 at 01:24:54AM +0530, Gyan Doshi wrote:
>>
>> On 2021-07-24 01:14, Michael Niedermayer wrote:
>>> On Fri, Jul 23, 2021 at 04:04:49PM +0530, Gyan Doshi wrote:
>>>> Currently, the user is forced to accept some noise in
>>>> packet payload, even if not requested.
>>>> ---
>>>>    doc/bitstream_filters.texi                    | 13 +++++-------
>>>>    libavcodec/noise_bsf.c                        | 21 +++++++++++++------
>>>>    tests/fate/matroska.mak                       |  2 +-
>>>>    .../fate/matroska-mastering-display-metadata  | 10 ++++-----
>>>>    4 files changed, 26 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>>>> index d10842ae47..2b84bda1fc 100644
>>>> --- a/doc/bitstream_filters.texi
>>>> +++ b/doc/bitstream_filters.texi
>>>> @@ -534,20 +534,17 @@ container. Can be used for fuzzing or testing error resilience/concealment.
>>>>    Parameters:
>>>>    @table @option
>>>>    @item amount
>>>> -A numeral string, whose value is related to how often output bytes will
>>>> -be modified. Therefore, values below or equal to 0 are forbidden, and
>>>> -the lower the more frequent bytes will be modified, with 1 meaning
>>>> -every byte is modified.
>>>> +Accepts a positive integer. Lower the value, more frequently bytes will be modified,
>>>> +with @var{1} meaning every byte is modified. Default is @var{0}.
>>>>    @item dropamount
>>>> -A numeral string, whose value is related to how often packets will be dropped.
>>>> -Therefore, values below or equal to 0 are forbidden, and the lower the more
>>>> -frequent packets will be dropped, with 1 meaning every packet is dropped.
>>>> +Accepts a positive integer. Lower the value, more frequently packets will be dropped,
>>>> +with @var{1} meaning every packet is dropped. Default is @var{0}.
>>>>    @end table
>>>>    The following example applies the modification to every byte but does not drop
>>>>    any packets.
>>>>    @example
>>>> -ffmpeg -i INPUT -c copy -bsf noise[=1] output.mkv
>>>> +ffmpeg -i INPUT -c copy -bsf noise=amount=1 output.mkv
>>>>    @end example
>>>>    @section null
>>>> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
>>>> index 6ebd369633..c1b0203442 100644
>>>> --- a/libavcodec/noise_bsf.c
>>>> +++ b/libavcodec/noise_bsf.c
>>>> @@ -33,20 +33,28 @@ typedef struct NoiseContext {
>>>>        unsigned int state;
>>>>    } NoiseContext;
>>>> -static int noise(AVBSFContext *ctx, AVPacket *pkt)
>>>> +static int noise_init(AVBSFContext *ctx)
>>>>    {
>>>>        NoiseContext *s = ctx->priv_data;
>>>> -    int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1);
>>>> -    int i, ret;
>>> you are droping the support for variable noise
>>>
>>> this also breaks a simple plain "-bsf noise"
>> Will re-add it. Is making it default required?
> what else would -bsf noise do ?

Originally, noise could only alter packet data, so it made sense that 
even with the default of amount=0. the filter shouldn't be a no-op, 
despite the intuitive semantics of amount=0 being no alteration .

Once dropping packets was added, there can't be an expectation that with 
amount=0. the user still wants to alter data. After all, the opt parser 
cannot distinguish between `-bsf noise` and `-bsf 
noise=amount=0[:dropamount=non-zero]`. This is how many other filters 
work - the default behaviour for setpts, scale, select, crop, pad..etc 
is a no-op not some arbitrary adjustment to the payload. That's not good UX.

Regards,
Gyan