[FFmpeg-devel] [PATCH 1/2] avfilter: Add fsync filter

[Thilo Borgmann]

Am 15.12.23 um 15:17 schrieb Andreas Rheinhardt:
> Thilo Borgmann via ffmpeg-devel:
>> ---
>>   Changelog                |   1 +
>>   MAINTAINERS              |   1 +
>>   doc/filters.texi         |  33 +++++
>>   libavfilter/Makefile     |   1 +
>>   libavfilter/allfilters.c |   1 +
>>   libavfilter/version.h    |   2 +-
>>   libavfilter/vf_fsync.c   | 304 +++++++++++++++++++++++++++++++++++++++
>>   7 files changed, 342 insertions(+), 1 deletion(-)
>>   create mode 100644 libavfilter/vf_fsync.c
>>
>> [...]
>> +// fills the buffer from cur to end, add \0 at EOF
>> +static int buf_fill(FsyncContext *ctx)
>> +{
>> +    int ret;
>> +    int num = ctx->end - ctx->cur;
>> +
>> +    ret = avio_read(ctx->avio_ctx, ctx->cur, num);
>> +    if (ret < 0)
>> +        return ret;
>> +    if (ret < num) {
>> +        *(ctx->cur + ret) = '\0';
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +// copies cur to end to the beginning and fills the rest
>> +static int buf_reload(FsyncContext *ctx)
>> +{
>> +    int i, ret;
>> +    int num = ctx->end - ctx->cur;
>> +
>> +    for (i = 0; i < num; i++) {
>> +        ctx->buf[i] = *ctx->cur++;
>> +    }
>> +
>> +    ctx->cur = ctx->buf + i;
>> +    ret = buf_fill(ctx);
>> +    if (ret < 0)
>> +        return ret;
>> +    ctx->cur = ctx->buf;
> 
> I wonder whether you should not just use avio_read_to_bprint() for all
> of this.

I tested a bit. It appears good for filling the buffer with one function call, getting the stuff out for scanf appears not as well as when and how to refill the buffer. Comparing a bit to f_metadata where this is used, the necessary function get bigger and more complex than this easily.
Maybe its just I didn't use it often enough, but this char* handling appears easier and less complex to me.
So if you don't insist I'd leave it that way.

>> +
>> +    return ret;
>> +}
>> +
>> +// skip from cur over eol
>> +static void buf_skip_eol(FsyncContext *ctx)
>> +{
>> +    char *i;
>> +    for (i = ctx->cur; i < ctx->end; i++) {
>> +        if (*i != '\n')// && *i != '\r')
>> +            break;
>> +    }
>> +    ctx->cur = i;
>> +}
>> +
>> +// get number of bytes from cur until eol
>> +static int buf_get_line_count(FsyncContext *ctx)
>> +{
>> +    int ret = 0;
>> +    char *i;
>> +    for (i = ctx->cur; i < ctx->end; i++, ret++) {
>> +        if (*i == '\0' || *i == '\n')
>> +            return ret;
> 
> If you unconditionally added a single \0 to the end of the buffer, you
> could use strchr() here.

I'd need two strchr() calls, for \0 and \n, plus the interpretation of the resulting pointers.
Where \0 would always be found at the end of the buffer which needs another if to see if its what I need or do need to load more data.


>> [...]
>> +
>> +static av_cold void fsync_uninit(AVFilterContext *ctx)
>> +{
>> +    FsyncContext *s = ctx->priv;
>> +
>> +    avio_close(s->avio_ctx);
> 
> avio_closep()
> 
>> +    av_freep(&s->buf);
>> +    av_frame_unref(s->last_frame);
> 
> I expect that this needs to be changed to av_frame_free(). Anyway, you
> should run your tests via valgrind/asan.

Asan is fine both ways, so I assume it wouldn't catch it?
Changed to av_frame_free anyways.

There appears no Valgrind nor msan to be available for OSX.
'leaks' also even complains it cannot really do its job:

"Can't examine target process's malloc zone asan_0x10c134950, so memory analysis will be incomplete or incorrect.
Reason: for security, cannot load non-system library /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/14.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib

Process 49784 is not debuggable. Due to security restrictions, leaks can only show or save contents of readonly memory of restricted processes."

Am I out of options on OSX? Seems like the first real drawback when on a mac...


Fixed all the other things for v3.

-Thilo