diff mbox series

[FFmpeg-devel] avfilter/avfilter: add sample_count_in and sample_count_out

Message ID 20200603133919.8560-1-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel] avfilter/avfilter: add sample_count_in and sample_count_out
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Paul B Mahol June 3, 2020, 1:39 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
To be used by latency filters.
---
 libavfilter/avfilter.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Nicolas George June 3, 2020, 3:11 p.m. UTC | #1
Paul B Mahol (12020-06-03):
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---

> To be used by latency filters.

Re-submit when something use them.

> ---
>  libavfilter/avfilter.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> index 49b4f7a939..8ebcb3eacc 100644
> --- a/libavfilter/avfilter.h
> +++ b/libavfilter/avfilter.h
> @@ -582,6 +582,11 @@ struct AVFilterLink {
>       */
>      int64_t frame_count_in, frame_count_out;
>  
> +    /**

> +     * Number of past samples sent through the link.

"Number of samples already sent through the link" seems more natural.

> +     */

> +    int64_t sample_count_in, sample_count_out;

Should be uint64_t.

> +
>      /**
>       * A pointer to a FFFramePool struct.
>       */
Paul B Mahol June 3, 2020, 3:33 p.m. UTC | #2
On 6/3/20, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12020-06-03):
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>
>> To be used by latency filters.
>
> Re-submit when something use them.

Why so? Just to be beaten again by same person which would reject this
same patch.

Low effort patches are better because author will not lose much time
in pointless
discussion with same person again and again. Instead patch is either
accepted or rejected
and than author can concentrate on more important stuff.

The idea behind this patch is to use those counters to report via
filter metadata current latency in filtergraph. Eg. (a)latency filter
would be inserted in filtergraph and then it would report latency
between some points in filtergraph, or just single filter before it.

If all reviewers are fond of this idea then OK, if not I have other
things to do and can not
lose precious time in timeless pointless discussion with same bitter
persons again and again.

>
>> ---
>>  libavfilter/avfilter.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
>> index 49b4f7a939..8ebcb3eacc 100644
>> --- a/libavfilter/avfilter.h
>> +++ b/libavfilter/avfilter.h
>> @@ -582,6 +582,11 @@ struct AVFilterLink {
>>       */
>>      int64_t frame_count_in, frame_count_out;
>>
>> +    /**
>
>> +     * Number of past samples sent through the link.
>
> "Number of samples already sent through the link" seems more natural.

I copy pasted sentence for frames above. Should it be changed too?

>
>> +     */
>
>> +    int64_t sample_count_in, sample_count_out;
>
> Should be uint64_t.

Above frame counters are not unsigned. Why to do different.

>
>> +
>>      /**
>>       * A pointer to a FFFramePool struct.
>>       */
>
> --
>   Nicolas George
>
Nicolas George June 3, 2020, 5:27 p.m. UTC | #3
Paul B Mahol (12020-06-03):
> Why so? Just to be beaten again by same person which would reject this
> same patch.

I was not the first one to reject a patch because in introduces APIs or
fields without users, but I think it is a good policy.

> > "Number of samples already sent through the link" seems more natural.
> I copy pasted sentence for frames above. Should it be changed too?

I do not think it matters much.
> 
> Above frame counters are not unsigned. Why to do different.

If I had noticed it when they were added, I would have suggested they be
unsigned too. Signed arithmetic is more tricky, and compiler optimize it
less well: if something does not need to be negative, let it be
unsigned.
James Almer June 3, 2020, 5:48 p.m. UTC | #4
On 6/3/2020 2:27 PM, Nicolas George wrote:
> Paul B Mahol (12020-06-03):
>> Why so? Just to be beaten again by same person which would reject this
>> same patch.
> 
> I was not the first one to reject a patch because in introduces APIs or
> fields without users, but I think it is a good policy.
> 
>>> "Number of samples already sent through the link" seems more natural.
>> I copy pasted sentence for frames above. Should it be changed too?
> 
> I do not think it matters much.
>>
>> Above frame counters are not unsigned. Why to do different.
> 
> If I had noticed it when they were added, I would have suggested they be
> unsigned too. Signed arithmetic is more tricky, and compiler optimize it
> less well: if something does not need to be negative, let it be
> unsigned.

None of these fields are public or meant to be accessed by any of the
other libraries, so they can be changed as needed. Feel free to make
them unsigned if you think it's worth it or more robust.
Just ensure you're not introducing bugs by doing so. A quick grep shows
things like FFMAX(0, outlink->frame_count_in - 1).
diff mbox series

Patch

diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
index 49b4f7a939..8ebcb3eacc 100644
--- a/libavfilter/avfilter.h
+++ b/libavfilter/avfilter.h
@@ -582,6 +582,11 @@  struct AVFilterLink {
      */
     int64_t frame_count_in, frame_count_out;
 
+    /**
+     * Number of past samples sent through the link.
+     */
+    int64_t sample_count_in, sample_count_out;
+
     /**
      * A pointer to a FFFramePool struct.
      */