[FFmpeg-devel] GSOC 2018 qualification task.

Michael Niedermayer michael at niedermayer.cc
Thu Apr 19 20:18:24 EEST 2018


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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180419/02883e06/attachment.sig>


More information about the ffmpeg-devel mailing list