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

Gyan Doshi ffmpeg at gyani.pro
Sat Jul 24 07:43:32 EEST 2021



On 2021-07-24 09:55, Andreas Rheinhardt wrote:
> 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.
>>
> Wrong: You can just set the default value of amount to -1 (meaning auto)
> which uses the current behaviour for zero. This is not an API change
> given that the doc disallowed setting amount to zero. Then one can allow
> (and document) setting amount to zero (in the sense that packets that
> are not dropped are unchanged).
I did intend to add -1 within the current range for amount. But I was 
talking about UX, not whether the current behaviour could be remapped to 
a new implicit default.

Regards,
Gyan


More information about the ffmpeg-devel mailing list