[FFmpeg-devel] libavcodec: add timer bitstream filter [v2]

[Andreas Håkon]

Hi Moritz,


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, 5 de August de 2019 17:31, Moritz Barsnick <barsnick at gmx.net> wrote:

> Hi Andreas,
>
> While trying to figure out how to add features I'd like to see into
> this ;-), some remarks:
>
> > +Apply an offset to the PTS/DTS timestamps.
> > +
> > + at table @option
> > + at item offset
> > +The offset value to apply to the PTS/DTS timestamps.
>
> For the unknowing, it might be useful to point out what sort of
> increments this is (i.e. not an actual "time" stamp or seconds).

The value are native PTS/DTS ticks, so a resolution of 90kHz.
So, you agree with something like this?

@item offset
"The absolute offset value to apply to the PTS/DTS timestamps
with a 90kHz resolution."


> [...]
> > +   { "offset", NULL, OFFSET(offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, FLAGS },
>
> Considering PTS/DTS is int64, wouldn't it be useful for the offset also
> being int64 (and using limits INT64_MIN, INT64_MAX)?

Aah, I feel you like then to full cover all the possible values.
Then two changes are required:

1. Use int64 types.
2. Limit offset values from -((2^33)-1) to +((2^33)-1)

You agree with this?


> > +// TODO: Instead of using absolute timestamp offsets, use frame numbers
>
> Alternatively? Or perhaps also AV_OPT_TYPE_DURATION? TODO is for later,
> anyway...

Perhaps I'll remove the TODO comments. The reason to appear is because some
other developers in the mailing-list have pointed some interesting ideas for
this plugin in the future. I'm not sure to remove it or not.


> > +   if (!s->first_debug_done) {
> > +          av_log(ctx, AV_LOG_DEBUG, "Updated PTS/DTS (%"PRId64"/%"PRId64" : %"PRId64"/%"PRId64") with offset:%d\\n",
> > +                  pkt->pts, pkt->dts, opts, odts, s->offset );
> > +   }
> > +   s->first_debug_done = 1;
>
> You should set the variable inside the if() block. otherwise it gets
> assigned on every packet.

Yes, a small optimization.


> > +static const AVClass timer_class = {
> > +   .class_name = "timer",
>
> In about half of the existing BSFs, this would be called "timer_bsf". I
> don't care, I just look at other existing code. ;-)
>

Good point! Other bitstream filters have this. I'll update it.
Thank you!


Regards.
A.H.

---