[FFmpeg-devel] GSOC 2018 qualification task.

Submitted by Michael Niedermayer on April 19, 2018, 5:18 p.m.

Details

Message ID 20180419171824.GL20131@michaelspb
State New
Headers show

Commit Message

Michael Niedermayer April 19, 2018, 5:18 p.m.
On Wed, Apr 18, 2018 at 02:45:36PM +0530, ANURAG SINGH IIT BHU wrote:
> Hello Sir,
> 
> I have implemented the suggested changes, now the filter does not break
> builds, also now it works for all inputs and

You are still removing the Copyright statments from the filter you copy


the newly added code has 960 lines
only 56 of these lines are not in vf_drawtext.c

diff -wbu libavfilter/vf_drawtext.c libavfilter/vf_hellosubs.c |diffstat 
 vf_hellosubs.c |  677 ++++-----------------------------------------------------
 1 file changed, 56 insertions(+), 621 deletions(-)

wc libavfilter/vf_hellosubs.c
  960  3333 32438 libavfilter/vf_hellosubs.c
  
From these 56 some are changes of the filter name and context name

The remaining changes are reviewed below, I attempted to format
this so the code is readable.


- * drawtext filter, based on the original vhook/drawtext.c
- * filter by Gustavo Sverzut Barbieri
+ * Libfreetype subtitles burning filter.
+ * @see{http://www.matroska.org/technical/specs/subtitles/ssa.html}

The SSA link has nothing to do with the code based on drawtext


@@ -207,3 +181,2 @@
-    {"text",        "set text",             OFFSET(text),               AV_OPT_TYPE_STRING, {.str=NULL},  CHAR_MIN, CHAR_MAX, FLAGS},
-    {"textfile",    "set text file",        OFFSET(textfile),           AV_OPT_TYPE_STRING, {.str=NULL},  CHAR_MIN, CHAR_MAX, FLAGS},
-    {"fontcolor",   "set foreground color", OFFSET(fontcolor.rgba),     AV_OPT_TYPE_COLOR,  {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS},
+    {"text",        "set text",             OFFSET(text),               AV_OPT_TYPE_STRING, {.str="Hello world"},  CHAR_MIN, CHAR_MAX, FLAGS},
+    {"fontcolor",   "set foreground color", OFFSET(fontcolor.rgba),     AV_OPT_TYPE_COLOR,  {.str="white"}, CHAR_MIN, CHAR_MAX, FLAGS},
@@ -217,5 +185,3 @@
-    {"fontsize",    "set font size",        OFFSET(fontsize_expr),      AV_OPT_TYPE_STRING, {.str=NULL},  CHAR_MIN, CHAR_MAX , FLAGS},
-    {"x",           "set x expression",     OFFSET(x_expr),             AV_OPT_TYPE_STRING, {.str="0"},   CHAR_MIN, CHAR_MAX, FLAGS},
-    {"y",           "set y expression",     OFFSET(y_expr),             AV_OPT_TYPE_STRING, {.str="0"},   CHAR_MIN, CHAR_MAX, FLAGS},
-    {"shadowx",     "set shadow x offset",  OFFSET(shadowx),            AV_OPT_TYPE_INT,    {.i64=0},     INT_MIN,  INT_MAX , FLAGS},
-    {"shadowy",     "set shadow y offset",  OFFSET(shadowy),            AV_OPT_TYPE_INT,    {.i64=0},     INT_MIN,  INT_MAX , FLAGS},
+    {"fontsize",    "set font size",        OFFSET(fontsize_expr),      AV_OPT_TYPE_STRING, {.str="h/20"},  CHAR_MIN, CHAR_MAX , FLAGS},
+    {"x",           "set x expression",     OFFSET(x_expr),             AV_OPT_TYPE_STRING, {.str="w/2.7"},   CHAR_MIN, CHAR_MAX, FLAGS},
+    {"y",           "set y expression",     OFFSET(y_expr),             AV_OPT_TYPE_STRING, {.str="h/1.3"},   CHAR_MIN, CHAR_MAX, FLAGS},

...

+static int generatehellosub(AVFilterContext *ctx, AVBPrint *bp)
 
 {
     DrawTextContext *s = ctx->priv;
     double pts = s->var_values[VAR_T];
     int64_t ms = llrint(pts * 1000);
 
+    if (ms < 0)
+       ms = -ms;
+    av_bprintf(bp, "Hello world %d:%02d",(int)(ms / (60 * 1000)),(int)(ms / 1000) % 60);
     return 0;
 }

The use of float/double can cause rounding diffferences between platforms
and is especially as the filters input is neverf float/double not needed

     if (s->fix_bounds) {
 
         /* calculate footprint of text effects */
-        int boxoffset     = s->draw_box ? FFMAX(s->boxborderw, 0) : 0;
-        int borderoffset  = s->borderw  ? FFMAX(s->borderw, 0) : 0;
 
-        int offsetleft = FFMAX3(boxoffset, borderoffset,
-                                (s->shadowx < 0 ? FFABS(s->shadowx) : 0));
-        int offsettop = FFMAX3(boxoffset, borderoffset,
-                                (s->shadowy < 0 ? FFABS(s->shadowy) : 0));
-
-        int offsetright = FFMAX3(boxoffset, borderoffset,
-                                 (s->shadowx > 0 ? s->shadowx : 0));
-        int offsetbottom = FFMAX3(boxoffset, borderoffset,
-                                  (s->shadowy > 0 ? s->shadowy : 0));
 
+        int offsetleft = FFMAX3(0,0,0);
+
+        int offsettop = FFMAX3(0, 0,0);
 
This can be simplified further


@@ -1515 +951 @@
-    .description   = NULL_IF_CONFIG_SMALL("Draw text on top of video frames using libfreetype library."),
+    .description   = NULL_IF_CONFIG_SMALL("Writes hello world time on top of video frames using libfreetype library."),

 
 

> 
> ffplay -f lavfi -i testsrc -vf hellosubs
> 
> works.
> 
> Sir I think passing the text using metadata to drawtext filter would not be
> an efficient way in terms of time as drawtext filter calls a number of
> functions which are not needed by the hellosubs filter, functions like
> expand_text(), expand_func() etc, i.e drawtext filter is of 1525 lines
> where as  hellosubs is more than 1/3 rd  times less in lines. and sir for
> realtime subtitles I think we should emphasise more on reducing the time
> taken by the filter to remove lag.

I think there are several misunderstandings here.

The code you submit is 960 lines of code but only about 56 lines
are not already in the codebase.
Its not an option to duplicate code like this if its intended to be pushed
into FFmpeg git. It also doesnt make the code faster.

Using metadata as i suggested would have solved this problem.
It was indeed not my first suggestion to solve this.
The first was to implent AV_MEDIA_TYPE_SUBTITLE in avfilter (this
would have been the best).

For synchronization of actual subtitles (not just hello world ones)
it will/would be needed to do proper synchronization. With AV_MEDIA_TYPE_SUBTITLE
that would come naturally from the timestamps of the frames.

Either way, this last patch was submitted after the deadline. So it doesnt 
affect your qualification task either way.

Thanks

[...]

Comments

ANURAG SINGH IIT BHU April 20, 2018, 5:05 a.m.
Hello Sir,

I do understand that just 56 lines were inserted but sir 621 lines were
deleted which were of no use for hellosubs and it included functons like
expand_text(), expand_function() and other which were being called by the
drawtext filter but not needed by the hellosubs filter as the text and
operation to perform was predetermined. So sir skipping unnecessary
functions should make the code a bit faster.

I am really looking forward to build my project proposal of speech to text
subtitle generation filter under your guidance and contribute to ffmpeg as
it will be helpful for a large number of users,and it is an important
opportunity for me as a student as well. I am dedicated to learn and I am
confident that with your guidance we can do it.


Thanks and regards,
Anurag Singh











‌

On Thu, Apr 19, 2018 at 10:48 PM, Michael Niedermayer <
michael@niedermayer.cc> wrote:

> On Wed, Apr 18, 2018 at 02:45:36PM +0530, ANURAG SINGH IIT BHU wrote:
> > Hello Sir,
> >
> > I have implemented the suggested changes, now the filter does not break
> > builds, also now it works for all inputs and
>
> You are still removing the Copyright statments from the filter you copy
>
> --- libavfilter/vf_drawtext.c   2018-04-17 14:20:30.340366781 +0200
> +++ libavfilter/vf_hellosubs.c  2018-04-19 17:51:48.371572589 +0200
> @@ -1,8 +1,4 @@
>  /*
> - * Copyright (c) 2011 Stefano Sabatini
> - * Copyright (c) 2010 S.N. Hemanth Meenakshisundaram
> - * Copyright (c) 2003 Gustavo Sverzut Barbieri <gsbarbieri@yahoo.com.br>
> - *
>   * This file is part of FFmpeg.
>   *
>   * FFmpeg is free software; you can redistribute it and/or
>
> the newly added code has 960 lines
> only 56 of these lines are not in vf_drawtext.c
>
> diff -wbu libavfilter/vf_drawtext.c libavfilter/vf_hellosubs.c |diffstat
>  vf_hellosubs.c |  677 ++++--------------------------
> ---------------------------
>  1 file changed, 56 insertions(+), 621 deletions(-)
>
> wc libavfilter/vf_hellosubs.c
>   960  3333 32438 libavfilter/vf_hellosubs.c
>
> From these 56 some are changes of the filter name and context name
>
> The remaining changes are reviewed below, I attempted to format
> this so the code is readable.
>
>
> - * drawtext filter, based on the original vhook/drawtext.c
> - * filter by Gustavo Sverzut Barbieri
> + * Libfreetype subtitles burning filter.
> + * @see{http://www.matroska.org/technical/specs/subtitles/ssa.html}
>
> The SSA link has nothing to do with the code based on drawtext
>
>
> @@ -207,3 +181,2 @@
> -    {"text",        "set text",             OFFSET(text),
>  AV_OPT_TYPE_STRING, {.str=NULL},  CHAR_MIN, CHAR_MAX, FLAGS},
> -    {"textfile",    "set text file",        OFFSET(textfile),
>  AV_OPT_TYPE_STRING, {.str=NULL},  CHAR_MIN, CHAR_MAX, FLAGS},
> -    {"fontcolor",   "set foreground color", OFFSET(fontcolor.rgba),
>  AV_OPT_TYPE_COLOR,  {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS},
> +    {"text",        "set text",             OFFSET(text),
>  AV_OPT_TYPE_STRING, {.str="Hello world"},  CHAR_MIN, CHAR_MAX, FLAGS},
> +    {"fontcolor",   "set foreground color", OFFSET(fontcolor.rgba),
>  AV_OPT_TYPE_COLOR,  {.str="white"}, CHAR_MIN, CHAR_MAX, FLAGS},
> @@ -217,5 +185,3 @@
> -    {"fontsize",    "set font size",        OFFSET(fontsize_expr),
> AV_OPT_TYPE_STRING, {.str=NULL},  CHAR_MIN, CHAR_MAX , FLAGS},
> -    {"x",           "set x expression",     OFFSET(x_expr),
>  AV_OPT_TYPE_STRING, {.str="0"},   CHAR_MIN, CHAR_MAX, FLAGS},
> -    {"y",           "set y expression",     OFFSET(y_expr),
>  AV_OPT_TYPE_STRING, {.str="0"},   CHAR_MIN, CHAR_MAX, FLAGS},
> -    {"shadowx",     "set shadow x offset",  OFFSET(shadowx),
> AV_OPT_TYPE_INT,    {.i64=0},     INT_MIN,  INT_MAX , FLAGS},
> -    {"shadowy",     "set shadow y offset",  OFFSET(shadowy),
> AV_OPT_TYPE_INT,    {.i64=0},     INT_MIN,  INT_MAX , FLAGS},
> +    {"fontsize",    "set font size",        OFFSET(fontsize_expr),
> AV_OPT_TYPE_STRING, {.str="h/20"},  CHAR_MIN, CHAR_MAX , FLAGS},
> +    {"x",           "set x expression",     OFFSET(x_expr),
>  AV_OPT_TYPE_STRING, {.str="w/2.7"},   CHAR_MIN, CHAR_MAX, FLAGS},
> +    {"y",           "set y expression",     OFFSET(y_expr),
>  AV_OPT_TYPE_STRING, {.str="h/1.3"},   CHAR_MIN, CHAR_MAX, FLAGS},
>
> ...
>
> +static int generatehellosub(AVFilterContext *ctx, AVBPrint *bp)
>
>  {
>      DrawTextContext *s = ctx->priv;
>      double pts = s->var_values[VAR_T];
>      int64_t ms = llrint(pts * 1000);
>
> +    if (ms < 0)
> +       ms = -ms;
> +    av_bprintf(bp, "Hello world %d:%02d",(int)(ms / (60 * 1000)),(int)(ms
> / 1000) % 60);
>      return 0;
>  }
>
> The use of float/double can cause rounding diffferences between platforms
> and is especially as the filters input is neverf float/double not needed
>
>      if (s->fix_bounds) {
>
>          /* calculate footprint of text effects */
> -        int boxoffset     = s->draw_box ? FFMAX(s->boxborderw, 0) : 0;
> -        int borderoffset  = s->borderw  ? FFMAX(s->borderw, 0) : 0;
>
> -        int offsetleft = FFMAX3(boxoffset, borderoffset,
> -                                (s->shadowx < 0 ? FFABS(s->shadowx) : 0));
> -        int offsettop = FFMAX3(boxoffset, borderoffset,
> -                                (s->shadowy < 0 ? FFABS(s->shadowy) : 0));
> -
> -        int offsetright = FFMAX3(boxoffset, borderoffset,
> -                                 (s->shadowx > 0 ? s->shadowx : 0));
> -        int offsetbottom = FFMAX3(boxoffset, borderoffset,
> -                                  (s->shadowy > 0 ? s->shadowy : 0));
>
> +        int offsetleft = FFMAX3(0,0,0);
> +
> +        int offsettop = FFMAX3(0, 0,0);
>
> This can be simplified further
>
>
> @@ -1515 +951 @@
> -    .description   = NULL_IF_CONFIG_SMALL("Draw text on top of video
> frames using libfreetype library."),
> +    .description   = NULL_IF_CONFIG_SMALL("Writes hello world time on top
> of video frames using libfreetype library."),
>
>
>
>
> >
> > ffplay -f lavfi -i testsrc -vf hellosubs
> >
> > works.
> >
> > Sir I think passing the text using metadata to drawtext filter would not
> be
> > an efficient way in terms of time as drawtext filter calls a number of
> > functions which are not needed by the hellosubs filter, functions like
> > expand_text(), expand_func() etc, i.e drawtext filter is of 1525 lines
> > where as  hellosubs is more than 1/3 rd  times less in lines. and sir for
> > realtime subtitles I think we should emphasise more on reducing the time
> > taken by the filter to remove lag.
>
> I think there are several misunderstandings here.
>
> The code you submit is 960 lines of code but only about 56 lines
> are not already in the codebase.
> Its not an option to duplicate code like this if its intended to be pushed
> into FFmpeg git. It also doesnt make the code faster.
>
> Using metadata as i suggested would have solved this problem.
> It was indeed not my first suggestion to solve this.
> The first was to implent AV_MEDIA_TYPE_SUBTITLE in avfilter (this
> would have been the best).
>
> For synchronization of actual subtitles (not just hello world ones)
> it will/would be needed to do proper synchronization. With
> AV_MEDIA_TYPE_SUBTITLE
> that would come naturally from the timestamps of the frames.
>
> Either way, this last patch was submitted after the deadline. So it doesnt
> affect your qualification task either way.
>
> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I know you won't believe me, but the highest form of Human Excellence is
> to question oneself and others. -- Socrates
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Michael Niedermayer April 27, 2018, 6:41 p.m.
Hi

On Fri, Apr 20, 2018 at 10:35:05AM +0530, ANURAG SINGH IIT BHU wrote:
> Hello Sir,
> 
> I do understand that just 56 lines were inserted but sir 621 lines were
> deleted which were of no use for hellosubs and it included functons like
> expand_text(), expand_function() and other which were being called by the
> drawtext filter but not needed by the hellosubs filter as the text and
> operation to perform was predetermined. So sir skipping unnecessary
> functions should make the code a bit faster.
> 

> I am really looking forward to build my project proposal of speech to text
> subtitle generation filter under your guidance and contribute to ffmpeg as
> it will be helpful for a large number of users,and it is an important
> opportunity for me as a student as well. I am dedicated to learn and I am
> confident that with your guidance we can do it.

If you want to work on this outside GSOC, you are certainly welcome to do so
and i would be happy to mentor it

Thanks

[...]

Patch hide | download patch | download mbox

--- libavfilter/vf_drawtext.c	2018-04-17 14:20:30.340366781 +0200
+++ libavfilter/vf_hellosubs.c	2018-04-19 17:51:48.371572589 +0200
@@ -1,8 +1,4 @@ 
 /*
- * Copyright (c) 2011 Stefano Sabatini
- * Copyright (c) 2010 S.N. Hemanth Meenakshisundaram
- * Copyright (c) 2003 Gustavo Sverzut Barbieri <gsbarbieri@yahoo.com.br>
- *
  * This file is part of FFmpeg.
  *
  * FFmpeg is free software; you can redistribute it and/or