[FFmpeg-devel] libavcodec: add timeshift bitstream filter [v3]

[Alexander Strasser]

Hi Nicolas!

On 2019-08-08 11:43 +0200, Nicolas George wrote:
> Andreas Håkon (12019-08-08):
> > > setpts or setts, similar to lavfi.
>
> > Then I proposse these two alternatives:
> >
> > 1) "editpts_bsf"
> > 2) "setpts_bsf"
> >
> > The "_bsf" is not real, only to differenciate (here) from other filters.
> > Which one do you prefer?
>
> editpts has no similarity with lavfi. Users would need to remember if it
> is filters <-> set and bsf <-> edit or filters <-> edit and bsf <-> set.
> Better have a single name.

Right, I thought about that too. It could get confusing. Still my initial
suggestions were on purpose not setpts. More on that below.


> Bitstream filters and lavfi are not in the same namespace, therefore the
> suffix is unneeded.
>
>
> Gyan (12019-08-08):
> > This BSF only allows to apply a fixed offset whereas the setpts filter
> > allows arbitrary expressions. Unless someone is planning to extend this
> > BSF,  the current candidate is more apt.

Like Gyan mentioned here, that was my reason to refrain from proposing
setpts as a name for this bsf. I too think, that one would expect being
able to use free expressions for the timestamp specification. And that
would cause similar confusions like the one you described above, where
users would think

    "ah, it's setpts I can just give it the offset I want"

vs

    "ah, it's setpts I can give it an expressions that evaluates
    to the time stamp I want to set"

Also if I'm not mistaken for the lavfi filter the name setpts is on
spot whereas for the bsf we would also deal with dts, which was not
a consideration when implementing the setpts filter and defining its
arguments, which is just a single argument named "expr".


> At some point, somebody will implement generic expression eval.

That might happen, but it's IMHO too vague to rely on for making
name decisions right in this moment.


> When
> that happens, it will be better if they can just add an option to the
> existing filter rather than creating a whole new filter, deprecating the
> existing one, etc.

That sounds a bit exaggerating to me:

1. Building a whole new bsf would not be that different from changing the
   existing one, except from having to copy and modify the boiler plate
   related to implementing bsfs in general. Maybe a couple of lines in
   the existing implementation of the bsf could be reused, maybe not.
2. The existing one wouldn't have to be deprecating, as it's so simple
   that it could stay there providing an alternative filter with a
   different interface and implementation. Or the code could be shared,
   which at the moment doesn't seem to have any benefits, but would also
   share the flaws for which there will be more opportunities in the bsf
   with expression evaluation.
3. If one would decide, one doesn't want to have the two achieving the
   same thing. One could replace the documentation of this bsf with
   "Only for backward compatibility. Use bsf setpts."

So yes it might be better if, but even then probably not much.

Also if it happens that all set filters of lavfi really get merged into
a single filter, we would have chosen the wrong name now.


> Key idea: long term planning.

I agree that long term planning is important, though I disagree with
your conclusion in this specific discussion.

The whole bsf framework is probably a nice long-term move. It's
flexibility of allowing bsfs as individual filters that are handled in
a generic fashion, provides a good ground for implementing a diverse,
useful and partially redundant set of bsfs.

Seeing it this way it eases long-term planning by removing the need for
detailed planning and allowing experimentation and evolution without
having to sacrifice compatibility.


> Therefore, I am for setpts (dts is just secondary), with a mention in
> the documentation:
>
> # Unlike the setpts filter in libavfilter, this filter currently can
> # only apply a constant offset.

Another name idea

* ts_offset

loosely in reference to -itsoffset and output_ts_offset (AVFormatContext).

And from the initial discussion there was:

* add_delay


Regarding setpts and other filters like addroi and so on, I find it a bit
unfortunate that they don't have an underscore. "set_" and "add_" is much
better for doing full text searches on the docs or the complete codebase.


If you still think setpts is the best name for that bsf, it's fine for
me; my main concern was to not name it "timer".


  Alexander