Thanks for the detailed review of the code, Paul. You caught some dumb
mistakes that I should have picked up on before I sent the submission.
> dynoverlay sounds very powerful, but this filter funcionality is very limited.
True, it is limited, but it does something that no other filter does (I am
interested in your named pipe idea, though -- see note at the end).
> What's up with code identation?
I don't know -- I didn't realize there was a problem. I'm just using vi; maybe
it formatted stuff in a funny way. Or maybe you don't like my braces on new
lines?
> > +If the named PNG file does not exist, the filter will do nothing.
>
> I do not like this design. Supporting only PNG with RGBA, working only
> in YUV420P.
I built the filter to satisfy a specific need; I thought that maybe others
who needed other formats could add in the support for those formats later.
I thought I saw some filters in ffmpeg that only support specific formats,
so I didn't realize that was a problem.
> > +@item check_interval
> > +(optional) The interval (in ms) between checks for updates to the overlay
> > file. For
> > +efficiency, the filter does not check the filesystem on every frame. You
> > can make
> > +it check more frequently (less efficient, but more responsive to changes in
> > the
> > +overlay PNG) by specifying a lower number. Or you can make it check less
> > frequently
> > +(more efficient, but less responsive to changes in the overlay PNG) by
> > specifying
> > +a higher number.
> > +
> > +Default value is @code{250}.
> > +@end table
>
> This approach is bad to provide such functionality.
Why? Yes, it's a lot of system calls, but it's still performant, so is
it fundamentally wrong?
> > + -vf format=pix_fmts=yuv420p \
> > + -vf dynoverlay=overlayfile=/var/tmp/overlay.png:check_interval=100 \
>
> You can not give more than one -vf's, only one will ever be used.
My mistake - I was trying to take a much more complicated example and distill it
down and anonymize it.
> > +#include <unistd.h>
> > ... many headers omitted ...
> > +#include "libavutil/lfg.h"
>
> Are all those headers really needed?
Probably not; I could try to pare it down.
> > + if (ctx->overlay_frame)
> > + {
> > + // TODO - make sure we have freed everything
> > + av_freep(&ctx->overlay_frame->data[0]);
> > + }
> > + ctx->overlay_frame = NULL;
>
> This is wrong.
Thanks for catching this; I should use av_frame_free(), right?
> > + if (ctx->overlay_frame)
> > + {
>
> Check is not needed.
>
> > + av_frame_free (&ctx->overlay_frame);
> > + }
> > + ctx->overlay_frame = NULL;
>
> Not needed.
Does this mean that it is safe to call av_frame_free() with NULL ?
> I think much better approach is to use named pipes, probably as
> separate video filter source.
I would love to see an example of this technique. Based on what I read online
(http://ffmpeg.gusari.org/viewtopic.php?f=11&t=2774 for example), I didn't
think this technique worked. And even if it could work, wouldn't I need a
process
that is constantly generating frame data from PNGs to feed it to ffmpeg, which
then has to read it 30 times a second (when it only changes every 2-3
*minutes*).
That seems extremely inefficient for overlays that are not changing frequently.
This is why I was trying to read the PNG once, hold it in memory until it
changes
or is removed. Maybe it would be more elegant if I wasn't polling the
filesystem;
I could send signals to ffmpeg instead. But I don't see a clear advantage to
that
technique, and there are some disadvantages. It's extremely simple for an
external
application to write to a specified file. But a model based on signalling would
require that the process also know the PID of the ffmpeg process.
I guess my question for you is whether this filter has any value to the larger
community. If not, I'll just maintain it myself as a patch that I can apply to
my own builds.
Jason Priebe
CBC New Media
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
