Re: [FFmpeg-devel] drawtext filter

[Paul B Mahol <onemda@gmail.com>]

On Tue, Mar 14, 2023 at 4:59 PM Francesco Carusi <klimklim@tiscali.it>
wrote:

> Can I help in any way in advancing this patch?
>

I will apply it if nobody objects in next 48h.


>
> On 03/02/2023 15:18, Francesco Carusi wrote:
> >
> >
> > On 30/01/2023 13:19, Paul B Mahol wrote:
> >> On 1/30/23, Francesco Carusi <klimklim@tiscali.it> wrote:
> >>> On 28/01/2023 16:32, Paul B Mahol wrote:
> >>>> On 1/28/23, Francesco Carusi <klimklim@tiscali.it> wrote:
> >>>>> On 27/01/2023 18:31, Paul B Mahol wrote:
> >>>>>> On 1/27/23, Francesco Carusi <klimklim@tiscali.it> wrote:
> >>>>>>> On 26/01/2023 17:37, Paul B Mahol wrote:
> >>>>>>>> On 1/26/23, Francesco Carusi <klimklim@tiscali.it> wrote:
> >>>>>>>>> On 26/01/2023 14:21, Paul B Mahol wrote:
> >>>>>>>>>> On 1/26/23, Francesco Carusi <klimklim@tiscali.it> wrote:
> >>>>>>>>>>> The drawtext reinit command is also used in the docs as an
> >>>>>>>>>>> example
> >>>>>>>>>>> for
> >>>>>>>>>>> the sendcmd filter, so I thought it was fine to use commands in
> >>>>>>>>>>> that
> >>>>>>>>>>> way. In my opinion it is also a convenient way to modify
> >>>>>>>>>>> multiple
> >>>>>>>>>>> options at the same time.
> >>>>>>>>>>> Should the command match the name of a filter option instead?
> >>>>>>>>>>>
> >>>>>>>>>> Please do not top post.
> >>>>>>>>>>
> >>>>>>>>>> It is much better to use already existing options for
> >>>>>>>>>> commands that
> >>>>>>>>>> is
> >>>>>>>>>> more intuitive to users. Also multiple options can be set at
> >>>>>>>>>> runtime,
> >>>>>>>>>> there is no such limitation.
> >>>>>>>>> ok, I'm going to remove the "change" command and add commands
> >>>>>>>>> that
> >>>>>>>>> match
> >>>>>>>>> the options that it included.
> >>>>>>>> Thanks, feel free to ask questions on #ffmpeg-devel irc channel.
> >>>>>>> I'm attaching the updated patch, I also updated the document at
> >>>>>>> https://github.com/yethie/FFmpeg/blob/master/drawtext/CHANGES.md
> >>>>>>> Thanks
> >>>>>> Amazing, I like demos!
> >>>>>>
> >>>>>> Could improve code style of newly added/changed lines?
> >>>>>> For example opening { put on separate line. So code style is in sync
> >>>>>> with rest of codebase.
> >>>>> Sure, I'll put opening { on a new line for functions, not for control
> >>>>> statements, like in the rest of the code. Is it fine?
> >>>> Yes. Thanks.
> >>>>
> >>>>>> The commands stuff does not need to use strcmp to detect if option
> >>>>>> value have been changed, you could avoid strcmp by just caching old
> >>>>>> value prior to calling function the picks new values, and after that
> >>>>>> just compare old with new and then if it differs call needed code.
> >>>>> I'll cache the numeric values. I think that caching string values
> >>>>> is not
> >>>>> the preferred solution because in addition to the strcmp needed to
> >>>>> check
> >>>>> the value, it would also need a strdup to cache the previous
> >>>>> value, even
> >>>>> when the command does not involve those options. Does it sound
> >>>>> reasonable?
> >>>> Yes.
> >>> I'm attaching the patch that includes the changes we discussed.
> >> space between 'for' and '('
> >>
> >> Do not keep old code in comments if its no longer relevant or working.
> >
> > Ok I added spaces between control statements (if, for, while) and '(',
> > and also cleaned up comments.
> > Following Anton Khirnov suggestion I tried to split the changes into
> > multiple commits. However the first one is quite bit since it contains
> > a major change in how the filter works and cannot be split further.
> > Patches attached.
> >
> >>>>>>>>>>> On 26/01/2023 11:50, Paul B Mahol wrote:
> >>>>>>>>>>>> On 1/26/23, Francesco Carusi <klimklim@tiscali.it> wrote:
> >>>>>>>>>>>>> Hi, I'm new to contributing to ffmpeg!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I modified the drawtext filter to improve text rendering
> >>>>>>>>>>>>> and add
> >>>>>>>>>>>>> some
> >>>>>>>>>>>>> features. You can find a high level description of the
> >>>>>>>>>>>>> changes
> >>>>>>>>>>>>> at
> >>>>>>>>>>>>> this
> >>>>>>>>>>>>> link:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> https://github.com/yethie/FFmpeg/blob/master/drawtext/CHANGES.md
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I'm also attaching the patch file.
> >>>>>>>>>>>>> I looked for the filter maintainer to discuss about the
> >>>>>>>>>>>>> changes
> >>>>>>>>>>>>> I
> >>>>>>>>>>>>> made
> >>>>>>>>>>>>> but it looks like there isn't any, am I correct?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Please let me know if this is the right way to submit my
> >>>>>>>>>>>>> contribution.
> >>>>>>>>>>>> Why filter can not support normal commands for options? Like
> >>>>>>>>>>>> most/all
> >>>>>>>>>>>> other filters that have support for changing options values at
> >>>>>>>>>>>> runtime.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The reinit and yours added change option(s) are very
> >>>>>>>>>>>> strange/inconvenient things to do.
> >>>>>>>>>>>> _______________________________________________
> >>>>>>>>>>>> ffmpeg-devel mailing list
> >>>>>>>>>>>> ffmpeg-devel@ffmpeg.org
> >>>>>>>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>>>>>>
> >>>>>>>>>>>> To unsubscribe, visit link above, or email
> >>>>>>>>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> ffmpeg-devel mailing list
> >>>>>>>>>>> ffmpeg-devel@ffmpeg.org
> >>>>>>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>>>>>
> >>>>>>>>>>> To unsubscribe, visit link above, or email
> >>>>>>>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>>>>>>>>
> >>>>>>>>>> _______________________________________________
> >>>>>>>>>> ffmpeg-devel mailing list
> >>>>>>>>>> ffmpeg-devel@ffmpeg.org
> >>>>>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>>>>
> >>>>>>>>>> To unsubscribe, visit link above, or email
> >>>>>>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>>>>>> _______________________________________________
> >>>>>>>>> ffmpeg-devel mailing list
> >>>>>>>>> ffmpeg-devel@ffmpeg.org
> >>>>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>>>
> >>>>>>>>> To unsubscribe, visit link above, or email
> >>>>>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>>>>>>
> >>>>>>>> _______________________________________________
> >>>>>>>> ffmpeg-devel mailing list
> >>>>>>>> ffmpeg-devel@ffmpeg.org
> >>>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>>>
> >>>>>>>> To unsubscribe, visit link above, or email
> >>>>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>>> _______________________________________________
> >>>>>> ffmpeg-devel mailing list
> >>>>>> ffmpeg-devel@ffmpeg.org
> >>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>
> >>>>>> To unsubscribe, visit link above, or email
> >>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>> _______________________________________________
> >>>>> ffmpeg-devel mailing list
> >>>>> ffmpeg-devel@ffmpeg.org
> >>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>
> >>>>> To unsubscribe, visit link above, or email
> >>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >>>>>
> >>>> _______________________________________________
> >>>> ffmpeg-devel mailing list
> >>>> ffmpeg-devel@ffmpeg.org
> >>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>
> >>>> To unsubscribe, visit link above, or email
> >>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".