[FFmpeg-devel] lavfi: add quirc filter

---
 Changelog                |   1 +
 configure                |   4 +
 doc/filters.texi         |  35 ++++++++
 libavfilter/Makefile     |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 225 insertions(+)
 create mode 100644 libavfilter/vf_quirc.c

Stefano Sabatini:
> ---
>  Changelog                |   1 +
>  configure                |   4 +
>  doc/filters.texi         |  35 ++++++++
>  libavfilter/Makefile     |   1 +
>  libavfilter/allfilters.c |   1 +
>  libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 225 insertions(+)
>  create mode 100644 libavfilter/vf_quirc.c
> 
> diff --git a/Changelog b/Changelog
> index b483bb9c69..424bfc11af 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -14,6 +14,7 @@ version <next>:
>  - D3D12VA hardware accelerated H264, HEVC, VP9, AV1, MPEG-2 and VC1 decoding
>  - tiltandshift filter
>  - qrencode filter and qrencodesrc source
> +- quirc filter
>  
>  version 6.1:
>  - libaribcaption decoder
> diff --git a/configure b/configure
> index 0b2f4e9792..419ff76bc1 100755
> --- a/configure
> +++ b/configure
> @@ -257,6 +257,7 @@ External library support:
>    --enable-libplacebo      enable libplacebo library [no]
>    --enable-libpulse        enable Pulseaudio input via libpulse [no]
>    --enable-libqrencode     enable QR encode generation via libqrencode [no]
> +  --enable-libquirc        enable QR decoding via libquirc [no]
>    --enable-librabbitmq     enable RabbitMQ library [no]
>    --enable-librav1e        enable AV1 encoding via rav1e [no]
>    --enable-librist         enable RIST via librist [no]
> @@ -1883,6 +1884,7 @@ EXTERNAL_LIBRARY_LIST="
>      libplacebo
>      libpulse
>      libqrencode
> +    libquirc
>      librabbitmq
>      librav1e
>      librist
> @@ -3793,6 +3795,7 @@ ocv_filter_deps="libopencv"
>  openclsrc_filter_deps="opencl"
>  qrencode_filter_deps="libqrencode"
>  qrencodesrc_filter_deps="libqrencode"
> +quirc_filter_deps="libquirc"
>  overlay_opencl_filter_deps="opencl"
>  overlay_qsv_filter_deps="libmfx"
>  overlay_qsv_filter_select="qsvvpp"
> @@ -6845,6 +6848,7 @@ enabled libopus           && {
>  enabled libplacebo        && require_pkg_config libplacebo "libplacebo >= 4.192.0" libplacebo/vulkan.h pl_vulkan_create
>  enabled libpulse          && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new
>  enabled libqrencode       && require_pkg_config libqrencode libqrencode qrencode.h QRcode_encodeString
> +enabled libquirc          && require libquirc quirc.h quirc_decode -lquirc
>  enabled librabbitmq       && require_pkg_config librabbitmq "librabbitmq >= 0.7.1" amqp.h amqp_new_connection
>  enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.5.0" rav1e.h rav1e_context_new
>  enabled librist           && require_pkg_config librist "librist >= 0.2.7" librist/librist.h rist_receiver_create
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 04898ede31..5fbe381d74 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -20368,6 +20368,41 @@ qrencode=text=%@{pts@}
>  
>  @end itemize
>  
> +@section quirc
> +
> +Identify and decode a QR code using the libquirc library (see
> +@url{https://github.com/dlbeer/quirc/}), and print the identified QR codes
> +positions and payload as metadata.
> +
> +To enable the compilation of this filter, you need to configure FFmpeg with with
> +@code{--enable-libquirc}.
> +
> +For each found QR code in the input video, some metadata entries are added with
> +the prefix @var{lavfi.quirc.N}, where @var{N} is the index, starting from 0,
> +associated to the QR code.
> +
> +A description of each metadata value follows:
> +
> +@table @option
> +@item lavfi.quirc.count
> +the number of found QR codes, it is not set in case none was found
> +
> +@item lavfi.quirc.N.x
> +@item lavfi.quirc.N.y
> +the top-left position of the @var{N} QR code
> +
> +@item lavfi.quirc.N.X
> +@item lavfi.quirc.N.Y
> +the bottom-right position of the @var{N} QR code
> +
> +@item lavfi.quirc.N.w
> +@item lavfi.quirc.N.h
> +the size of the @var{N} QR code
> +
> +@item lavfi.quirc.N.payload
> +the payload of the QR code
> +@end table
> +
>  @section random
>  
>  Flush video frames from internal cache of frames into a random order.
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 31371ceb1a..f65fb9a5a7 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -437,6 +437,7 @@ OBJS-$(CONFIG_PSEUDOCOLOR_FILTER)            += vf_pseudocolor.o
>  OBJS-$(CONFIG_PSNR_FILTER)                   += vf_psnr.o framesync.o
>  OBJS-$(CONFIG_PULLUP_FILTER)                 += vf_pullup.o
>  OBJS-$(CONFIG_QP_FILTER)                     += vf_qp.o
> +OBJS-$(CONFIG_QUIRC_FILTER)                  += vf_quirc.o
>  OBJS-$(CONFIG_RANDOM_FILTER)                 += vf_random.o
>  OBJS-$(CONFIG_READEIA608_FILTER)             += vf_readeia608.o
>  OBJS-$(CONFIG_READVITC_FILTER)               += vf_readvitc.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 20feb37967..b8570dbab2 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -412,6 +412,7 @@ extern const AVFilter ff_vf_psnr;
>  extern const AVFilter ff_vf_pullup;
>  extern const AVFilter ff_vf_qp;
>  extern const AVFilter ff_vf_qrencode;
> +extern const AVFilter ff_vf_quirc;
>  extern const AVFilter ff_vf_random;
>  extern const AVFilter ff_vf_readeia608;
>  extern const AVFilter ff_vf_readvitc;
> diff --git a/libavfilter/vf_quirc.c b/libavfilter/vf_quirc.c
> new file mode 100644
> index 0000000000..6945a6aa31
> --- /dev/null
> +++ b/libavfilter/vf_quirc.c
> @@ -0,0 +1,183 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file QR decoder video filter
> + *
> + * Use libquirc library to decode the content of QR codes, and put the decoded
> + * content to metadata. See:
> + * https://github.com/dlbeer/quirc
> + */
> +
> +#include "config_components.h"

Unneeded.

> +//#include "libavutil/internal.h"
> +#include "libavutil/opt.h"
> +#include "avfilter.h"
> +#include "formats.h"
> +#include "video.h"
> +#include <quirc.h>
> +
> +typedef struct QuircContext {
> +    const AVClass *class;
> +
> +    struct quirc *quirc;
> +} QuircContext;
> +
> +static const AVOption quirc_options[] = {
> +    { NULL }
> +};

There is no need for empty options (and also no need for an AVClass for
a filter without options).

> +
> +static av_cold int init(AVFilterContext *ctx)
> +{
> +    QuircContext *quirc = ctx->priv;
> +
> +    quirc->quirc = quirc_new();
> +    if (!quirc->quirc) {
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    return 0;
> +}
> +
> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> +    QuircContext *quirc = ctx->priv;
> +
> +    quirc_destroy(quirc->quirc);
> +}
> +
> +static int config_input(AVFilterLink *inlink)
> +{
> +    AVFilterContext *ctx = inlink->dst;
> +    QuircContext *quirc = ctx->priv;
> +    int err;
> +
> +    err = quirc_resize(quirc->quirc, inlink->w, inlink->h);
> +    if (err == -1) {
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    return 0;
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    static const enum AVPixelFormat pix_fmts[] = {AV_PIX_FMT_GRAY8, AV_PIX_FMT_NONE};
> +    return ff_set_common_formats_from_list(ctx, pix_fmts);
> +}

FILTER_QUERY_FUNC is inappropriate for this.

> +
> +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> +{
> +    AVFilterContext *ctx = inlink->dst;
> +    AVFilterLink *outlink = ctx->outputs[0];
> +    QuircContext *quirc = ctx->priv;
> +    int codes_count;
> +    int i, w, h;
> +    uint8_t *image, *imagep, *srcp;
> +
> +    image = quirc_begin(quirc->quirc, &w, &h);
> +
> +    /* copy input image to quirc buffer */
> +    imagep = image;
> +    srcp = frame->data[0];
> +    for (i = 0; i < inlink->h; i++) {
> +        memcpy(imagep, srcp, w);
> +        imagep += w;
> +        srcp += frame->linesize[0];
> +    }
> +
> +    quirc_end(quirc->quirc);
> +
> +    codes_count = quirc_count(quirc->quirc);
> +    av_log(ctx, AV_LOG_VERBOSE,
> +           "Found count %d codes in image #%ld\n", codes_count, inlink->frame_count_out);
> +
> +    if (codes_count) {
> +        AVDictionary **metadata = &frame->metadata;
> +
> +        av_dict_set_int(metadata, "lavfi.quirc.count", codes_count, 0);
> +
> +        for (i = 0; i < codes_count; i++) {
> +            struct quirc_code code;
> +            struct quirc_data data;
> +            struct quirc_point top_left, bottom_right;
> +            quirc_decode_error_t err;
> +            int w, h;
> +            char metadata_key[64];
> +
> +            quirc_extract(quirc->quirc, i, &code);
> +
> +            err = quirc_decode(&code, &data);
> +            if (err) {
> +                av_log(ctx, AV_LOG_WARNING,
> +                       "Failed to decode image: %s\n", quirc_strerror(err));
> +                continue;
> +            }
> +
> +            top_left = code.corners[0];
> +            bottom_right = code.corners[2];
> +            w = bottom_right.x - top_left.x;
> +            h = bottom_right.y - top_left.y;
> +
> +#define SET_METADATA_I(key_, value_)                                    \
> +            snprintf(metadata_key, sizeof(metadata_key)-1, "lavfi.quirc.%d." #key_, i); \
> +            av_dict_set_int(metadata, metadata_key, value_, AV_DICT_MATCH_CASE)
> +
> +            SET_METADATA_I(x, top_left.x);
> +            SET_METADATA_I(y, top_left.y);
> +            SET_METADATA_I(X, bottom_right.x);
> +            SET_METADATA_I(Y, bottom_right.y);
> +            SET_METADATA_I(w, w);
> +            SET_METADATA_I(h, h);
> +            snprintf(metadata_key, sizeof(metadata_key)-1, "lavfi.quirc.%d.payload", i); \
> +            av_dict_set(metadata, metadata_key, data.payload, 0);
> +
> +            av_log(ctx, AV_LOG_INFO,
> +                   "Found QR code at position %d+%d:%dx%d with payload: %s\n",
> +                   top_left.x, top_left.y, w, h, data.payload);
> +        }
> +    }
> +
> +    return ff_filter_frame(outlink, frame);
> +}
> +
> +AVFILTER_DEFINE_CLASS(quirc);
> +
> +static const AVFilterPad inputs[] = {
> +    {
> +        .name         = "default",
> +        .type         = AVMEDIA_TYPE_VIDEO,
> +        .filter_frame = filter_frame,
> +        .config_props = config_input
> +    },
> +};
> +
> +const AVFilter ff_vf_quirc = {
> +    .name        = "quirc",
> +    .description = NULL_IF_CONFIG_SMALL("Decode and show QR codes content."),
> +    .priv_size   = sizeof(QuircContext),
> +    .priv_class  = &quirc_class,
> +    .init        = init,
> +    .uninit      = uninit,
> +    FILTER_INPUTS(inputs),
> +    FILTER_OUTPUTS(ff_video_default_filterpad),
> +    FILTER_QUERY_FUNC(query_formats),
> +    .flags       = AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC |
> +                   AVFILTER_FLAG_METADATA_ONLY,
> +};
> +

On 12/26/2023 1:20 PM, Stefano Sabatini wrote:
> +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> +{
> +    AVFilterContext *ctx = inlink->dst;
> +    AVFilterLink *outlink = ctx->outputs[0];
> +    QuircContext *quirc = ctx->priv;
> +    int codes_count;
> +    int i, w, h;
> +    uint8_t *image, *imagep, *srcp;
> +
> +    image = quirc_begin(quirc->quirc, &w, &h);

Can't this fail? Or does the allocation take place in the quirc_resize() 
call?

> +
> +    /* copy input image to quirc buffer */
> +    imagep = image;
> +    srcp = frame->data[0];
> +    for (i = 0; i < inlink->h; i++) {
> +        memcpy(imagep, srcp, w);

Can w differ from inlink->w?

> +        imagep += w;
> +        srcp += frame->linesize[0];
> +    }

Maybe instead do

av_image_copy_plane(image, w, frame->data[0], frame->linesize[0],
                     w, inlink->h);

On date Tuesday 2023-12-26 16:21:55 -0300, James Almer wrote:
> On 12/26/2023 1:20 PM, Stefano Sabatini wrote:
> > +static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> > +{
> > +    AVFilterContext *ctx = inlink->dst;
> > +    AVFilterLink *outlink = ctx->outputs[0];
> > +    QuircContext *quirc = ctx->priv;
> > +    int codes_count;
> > +    int i, w, h;
> > +    uint8_t *image, *imagep, *srcp;
> > +
> > +    image = quirc_begin(quirc->quirc, &w, &h);
> 

> Can't this fail?

> Or does the allocation take place in the quirc_resize()
> call?

correct

> > +
> > +    /* copy input image to quirc buffer */
> > +    imagep = image;
> > +    srcp = frame->data[0];
> > +    for (i = 0; i < inlink->h; i++) {
> > +        memcpy(imagep, srcp, w);
> 

> Can w differ from inlink->w?

No, given that the resize is done in config_inputs.


> > +        imagep += w;
> > +        srcp += frame->linesize[0];
> > +    }
> 
> Maybe instead do
> 

> av_image_copy_plane(image, w, frame->data[0], frame->linesize[0],
>                     w, inlink->h);

good catch, thanks for the feedback

On date Tuesday 2023-12-26 20:08:10 +0100, Andreas Rheinhardt wrote:
> Stefano Sabatini:
[...]
> > +static int query_formats(AVFilterContext *ctx)
> > +{
> > +    static const enum AVPixelFormat pix_fmts[] = {AV_PIX_FMT_GRAY8, AV_PIX_FMT_NONE};
> > +    return ff_set_common_formats_from_list(ctx, pix_fmts);
> > +}
> 
> FILTER_QUERY_FUNC is inappropriate for this.

What should I use instead?

On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini wrote:
> ---
>  Changelog                |   1 +
>  configure                |   4 +
>  doc/filters.texi         |  35 ++++++++
>  libavfilter/Makefile     |   1 +
>  libavfilter/allfilters.c |   1 +
>  libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 225 insertions(+)
>  create mode 100644 libavfilter/vf_quirc.c

V2 with a few fixes and all corners put in the metadata (e.g. in case
the QR code is rotated).

On 12/26/2023 10:25 PM, Stefano Sabatini wrote:
> On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini wrote:
>> ---
>>   Changelog                |   1 +
>>   configure                |   4 +
>>   doc/filters.texi         |  35 ++++++++
>>   libavfilter/Makefile     |   1 +
>>   libavfilter/allfilters.c |   1 +
>>   libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
>>   6 files changed, 225 insertions(+)
>>   create mode 100644 libavfilter/vf_quirc.c
> 
> V2 with a few fixes and all corners put in the metadata (e.g. in case
> the QR code is rotated).

Looking at the library, the license is very permissive and the code 
hasn't been touched in many years. It is also pretty small, so why not 
just add it as a native filter instead of requiring an external 
dependency for what seems to be a relatively simple process?

On date Tuesday 2023-12-26 22:33:36 -0300, James Almer wrote:
> On 12/26/2023 10:25 PM, Stefano Sabatini wrote:
> > On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini wrote:
> > > ---
> > >   Changelog                |   1 +
> > >   configure                |   4 +
> > >   doc/filters.texi         |  35 ++++++++
> > >   libavfilter/Makefile     |   1 +
> > >   libavfilter/allfilters.c |   1 +
> > >   libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
> > >   6 files changed, 225 insertions(+)
> > >   create mode 100644 libavfilter/vf_quirc.c
> > 
> > V2 with a few fixes and all corners put in the metadata (e.g. in case
> > the QR code is rotated).
> 

> Looking at the library, the license is very permissive and the code hasn't
> been touched in many years. It is also pretty small, so why not just add it
> as a native filter instead of requiring an external dependency for what
> seems to be a relatively simple process?

I see pros and cons, in total that would be about 3K lines of pretty
clean code and data, and this would simplify integration for end-users
(since they would not need to build the library, which seems not
packaged by many distributions), and having the code would help to
solve similar problems and probably could be generalized and optimized
(e.g. to support other pixel formats).

OTOH it would add to the maintenance burden since we would be owners
of the code, which also means we would not benefit from fixes to the
upstream project, in case they happen (last commit is from March
2023, so not very old):
https://github.com/dlbeer/quirc/commit/542848dd6b9b0eaa9587bbf25b9bc67bd8a71fca

ons 2023-12-27 klockan 12:37 +0100 skrev Stefano Sabatini:
> On date Tuesday 2023-12-26 22:33:36 -0300, James Almer wrote:
> > On 12/26/2023 10:25 PM, Stefano Sabatini wrote:
> > > On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini
> > > wrote:
> > > > ---
> > > >   Changelog                |   1 +
> > > >   configure                |   4 +
> > > >   doc/filters.texi         |  35 ++++++++
> > > >   libavfilter/Makefile     |   1 +
> > > >   libavfilter/allfilters.c |   1 +
> > > >   libavfilter/vf_quirc.c   | 183
> > > > +++++++++++++++++++++++++++++++++++++++
> > > >   6 files changed, 225 insertions(+)
> > > >   create mode 100644 libavfilter/vf_quirc.c
> > > 
> > > V2 with a few fixes and all corners put in the metadata (e.g. in
> > > case
> > > the QR code is rotated).
> > 
> 
> > Looking at the library, the license is very permissive and the code
> > hasn't
> > been touched in many years. It is also pretty small, so why not
> > just add it
> > as a native filter instead of requiring an external dependency for
> > what
> > seems to be a relatively simple process?
> 
> I see pros and cons, in total that would be about 3K lines of pretty
> clean code and data, and this would simplify integration for end-
> users
> (since they would not need to build the library, which seems not
> packaged by many distributions), and having the code would help to
> solve similar problems and probably could be generalized and
> optimized
> (e.g. to support other pixel formats).
> 
> OTOH it would add to the maintenance burden since we would be owners
> of the code, which also means we would not benefit from fixes to the
> upstream project, in case they happen (last commit is from March
> 2023, so not very old):
> https://github.com/dlbeer/quirc/commit/542848dd6b9b0eaa9587bbf25b9bc67bd8a71fca

We could use git-subtree to keep in sync with upstream and to
contribute patches back. This is something I've done with other
projects. It has the following benefits:

1) Not having to deal with git-submodule
2) Having a copy of the code in case upstream goes down
3) Possibility of applying FFmpeg-only changes, say to the build
system, without having to have scripts that apply patches on top of the
submodule
4) Development can be concentrated on one implementation rather than
many. This is a point I've made with MXF but it applies here too

However, a bit of work is necessary to make packagers happy with such
an arrangeent, since I expect they would want libquirc to be a separate
package.

On the other hand we can just force users to build and install
libquirc. This is less work for us.

/Tomas

On 12/27/2023 8:37 AM, Stefano Sabatini wrote:
> On date Tuesday 2023-12-26 22:33:36 -0300, James Almer wrote:
>> On 12/26/2023 10:25 PM, Stefano Sabatini wrote:
>>> On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini wrote:
>>>> ---
>>>>    Changelog                |   1 +
>>>>    configure                |   4 +
>>>>    doc/filters.texi         |  35 ++++++++
>>>>    libavfilter/Makefile     |   1 +
>>>>    libavfilter/allfilters.c |   1 +
>>>>    libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
>>>>    6 files changed, 225 insertions(+)
>>>>    create mode 100644 libavfilter/vf_quirc.c
>>>
>>> V2 with a few fixes and all corners put in the metadata (e.g. in case
>>> the QR code is rotated).
>>
> 
>> Looking at the library, the license is very permissive and the code hasn't
>> been touched in many years. It is also pretty small, so why not just add it
>> as a native filter instead of requiring an external dependency for what
>> seems to be a relatively simple process?
> 
> I see pros and cons, in total that would be about 3K lines of pretty
> clean code and data, and this would simplify integration for end-users
> (since they would not need to build the library, which seems not
> packaged by many distributions), and having the code would help to
> solve similar problems and probably could be generalized and optimized
> (e.g. to support other pixel formats).
> 
> OTOH it would add to the maintenance burden since we would be owners
> of the code, which also means we would not benefit from fixes to the
> upstream project, in case they happen (last commit is from March
> 2023, so not very old):
> https://github.com/dlbeer/quirc/commit/542848dd6b9b0eaa9587bbf25b9bc67bd8a71fca

That's a build system change, so not really relevant. Last real change 
was 
https://github.com/dlbeer/quirc/commit/cc673124335785d220dbb9057b21c51e4a87e0b2, 
also from March 2023, but the one before it is from August 2021.

I really think this should be ported as a native filter. It's not big 
and complex like a scaler (sws, zscale) which should not live within 
libavfilter. Any change can be contributed back to libquirc (A library 
that's not going to be abandoned like it happened with libdcadec after 
it was merged into lavc, because its license is more permissive than our 
LGPL, which has its merits).

Lets try to keep external dependencies as limited as possible.

On date Wednesday 2023-12-27 14:16:24 -0300, James Almer wrote:
> On 12/27/2023 8:37 AM, Stefano Sabatini wrote:
> > On date Tuesday 2023-12-26 22:33:36 -0300, James Almer wrote:
> > > On 12/26/2023 10:25 PM, Stefano Sabatini wrote:
> > > > On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini wrote:
> > > > > ---
> > > > >    Changelog                |   1 +
> > > > >    configure                |   4 +
> > > > >    doc/filters.texi         |  35 ++++++++
> > > > >    libavfilter/Makefile     |   1 +
> > > > >    libavfilter/allfilters.c |   1 +
> > > > >    libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
> > > > >    6 files changed, 225 insertions(+)
> > > > >    create mode 100644 libavfilter/vf_quirc.c
> > > > 
> > > > V2 with a few fixes and all corners put in the metadata (e.g. in case
> > > > the QR code is rotated).
> > > 
> > 
> > > Looking at the library, the license is very permissive and the code hasn't
> > > been touched in many years. It is also pretty small, so why not just add it
> > > as a native filter instead of requiring an external dependency for what
> > > seems to be a relatively simple process?
> > 
> > I see pros and cons, in total that would be about 3K lines of pretty
> > clean code and data, and this would simplify integration for end-users
> > (since they would not need to build the library, which seems not
> > packaged by many distributions), and having the code would help to
> > solve similar problems and probably could be generalized and optimized
> > (e.g. to support other pixel formats).
> > 
> > OTOH it would add to the maintenance burden since we would be owners
> > of the code, which also means we would not benefit from fixes to the
> > upstream project, in case they happen (last commit is from March
> > 2023, so not very old):
> > https://github.com/dlbeer/quirc/commit/542848dd6b9b0eaa9587bbf25b9bc67bd8a71fca
> 
> That's a build system change, so not really relevant. Last real change was https://github.com/dlbeer/quirc/commit/cc673124335785d220dbb9057b21c51e4a87e0b2,
> also from March 2023, but the one before it is from August 2021.
> 

> I really think this should be ported as a native filter. It's not big and
> complex like a scaler (sws, zscale) which should not live within
> libavfilter.

> Any change can be contributed back to libquirc

This is not realistic, given that a reimplementation would be possibly
completely refactored to fit into FFmpeg.

> (A library
> that's not going to be abandoned like it happened with libdcadec after it
> was merged into lavc,

OTOH, this library is quite outside the scope of the FFmpeg, so it
makes sense to keep it as external dependency. This is a quite
different use case than a decoder, a QR-decoder library can make sense
outside of a multimedia library, for a decoder you would need a
complete multimedia library anyway.

I was checking the code, and porting would be a serious effort and
comprise several thousands lines of code (against the moderate effort
of wrapping it - which is already done), also some of the logic would
not really fit into FFmpeg because it is quite specific to this
application domain (QR code decoding), so it makes sense for it to
live within an external library. Not to mention that this would be
a duplication of effort.

*Unless* someone is willing to port/reimplement the code, but this
should not be a blocker for the wrapper and can be done as a separate
step.

On 12/27/2023 8:20 PM, Stefano Sabatini wrote:
> On date Wednesday 2023-12-27 14:16:24 -0300, James Almer wrote:
>> On 12/27/2023 8:37 AM, Stefano Sabatini wrote:
>>> On date Tuesday 2023-12-26 22:33:36 -0300, James Almer wrote:
>>>> On 12/26/2023 10:25 PM, Stefano Sabatini wrote:
>>>>> On date Tuesday 2023-12-26 17:20:33 +0100, Stefano Sabatini wrote:
>>>>>> ---
>>>>>>     Changelog                |   1 +
>>>>>>     configure                |   4 +
>>>>>>     doc/filters.texi         |  35 ++++++++
>>>>>>     libavfilter/Makefile     |   1 +
>>>>>>     libavfilter/allfilters.c |   1 +
>>>>>>     libavfilter/vf_quirc.c   | 183 +++++++++++++++++++++++++++++++++++++++
>>>>>>     6 files changed, 225 insertions(+)
>>>>>>     create mode 100644 libavfilter/vf_quirc.c
>>>>>
>>>>> V2 with a few fixes and all corners put in the metadata (e.g. in case
>>>>> the QR code is rotated).
>>>>
>>>
>>>> Looking at the library, the license is very permissive and the code hasn't
>>>> been touched in many years. It is also pretty small, so why not just add it
>>>> as a native filter instead of requiring an external dependency for what
>>>> seems to be a relatively simple process?
>>>
>>> I see pros and cons, in total that would be about 3K lines of pretty
>>> clean code and data, and this would simplify integration for end-users
>>> (since they would not need to build the library, which seems not
>>> packaged by many distributions), and having the code would help to
>>> solve similar problems and probably could be generalized and optimized
>>> (e.g. to support other pixel formats).
>>>
>>> OTOH it would add to the maintenance burden since we would be owners
>>> of the code, which also means we would not benefit from fixes to the
>>> upstream project, in case they happen (last commit is from March
>>> 2023, so not very old):
>>> https://github.com/dlbeer/quirc/commit/542848dd6b9b0eaa9587bbf25b9bc67bd8a71fca
>>
>> That's a build system change, so not really relevant. Last real change was https://github.com/dlbeer/quirc/commit/cc673124335785d220dbb9057b21c51e4a87e0b2,
>> also from March 2023, but the one before it is from August 2021.
>>
> 
>> I really think this should be ported as a native filter. It's not big and
>> complex like a scaler (sws, zscale) which should not live within
>> libavfilter.
> 
>> Any change can be contributed back to libquirc
> 
> This is not realistic, given that a reimplementation would be possibly
> completely refactored to fit into FFmpeg.
> 
>> (A library
>> that's not going to be abandoned like it happened with libdcadec after it
>> was merged into lavc,
> 
> OTOH, this library is quite outside the scope of the FFmpeg, so it
> makes sense to keep it as external dependency. This is a quite
> different use case than a decoder, a QR-decoder library can make sense
> outside of a multimedia library, for a decoder you would need a
> complete multimedia library anyway.
> 
> I was checking the code, and porting would be a serious effort and
> comprise several thousands lines of code (against the moderate effort
> of wrapping it - which is already done), also some of the logic would
> not really fit into FFmpeg because it is quite specific to this
> application domain (QR code decoding), so it makes sense for it to
> live within an external library. Not to mention that this would be
> a duplication of effort.

Image analysis is within FFmpeg's scope, which QR code identification 
and decoding would be about.

> 
> *Unless* someone is willing to port/reimplement the code, but this
> should not be a blocker for the wrapper and can be done as a separate
> step.

No, i'm not blocking anything. Just stating that ideally this would be a 
native module.

tor 2023-12-28 klockan 00:20 +0100 skrev Stefano Sabatini:
> I was checking the code, and porting would be a serious effort and
> comprise several thousands lines of code (against the moderate effort
> of wrapping it - which is already done), also some of the logic would
> not really fit into FFmpeg because it is quite specific to this
> application domain (QR code decoding), so it makes sense for it to
> live within an external library. Not to mention that this would be
> a duplication of effort.

This is the same point I've been making lately. With an appropriate
build system a lot of effort can be saved. It's only really when
existing implementations are too slow or mising features that it might
be necessary to reimplement them.

/Tomas

On date Wednesday 2023-12-27 21:57:19 -0300, James Almer wrote:
> On 12/27/2023 8:20 PM, Stefano Sabatini wrote:
[...]
> > > I really think this should be ported as a native filter. It's not big and
> > > complex like a scaler (sws, zscale) which should not live within
> > > libavfilter.
> > 
> > > Any change can be contributed back to libquirc
> > 
> > This is not realistic, given that a reimplementation would be possibly
> > completely refactored to fit into FFmpeg.
> > 
> > > (A library
> > > that's not going to be abandoned like it happened with libdcadec after it
> > > was merged into lavc,
> > 
> > OTOH, this library is quite outside the scope of the FFmpeg, so it
> > makes sense to keep it as external dependency. This is a quite
> > different use case than a decoder, a QR-decoder library can make sense
> > outside of a multimedia library, for a decoder you would need a
> > complete multimedia library anyway.
> > 
> > I was checking the code, and porting would be a serious effort and
> > comprise several thousands lines of code (against the moderate effort
> > of wrapping it - which is already done), also some of the logic would
> > not really fit into FFmpeg because it is quite specific to this
> > application domain (QR code decoding), so it makes sense for it to
> > live within an external library. Not to mention that this would be
> > a duplication of effort.
> 
> Image analysis is within FFmpeg's scope, which QR code identification and
> decoding would be about.
> 
> > 
> > *Unless* someone is willing to port/reimplement the code, but this
> > should not be a blocker for the wrapper and can be done as a separate
> > step.
> 
> No, i'm not blocking anything. Just stating that ideally this would be a
> native module.

I'm not against this of course, but as I already stated this would
require more effort. It is probably worth it (polynomials and Galois
fields based error correction) as they would be probably reused in
other parts of the code. Ditto for the QR encoder.

But I plan to have this functionality in with the wrappers - this
should also help with an eventual native port of the features.

I plan to push this patch in a few days, unless I see comments.

Thanks for the feedback.

On date Friday 2023-12-29 12:57:15 +0100, Stefano Sabatini wrote:
> On date Wednesday 2023-12-27 21:57:19 -0300, James Almer wrote:
> > On 12/27/2023 8:20 PM, Stefano Sabatini wrote:
> [...]
> > > > I really think this should be ported as a native filter. It's not big and
> > > > complex like a scaler (sws, zscale) which should not live within
> > > > libavfilter.
> > > 
> > > > Any change can be contributed back to libquirc
> > > 
> > > This is not realistic, given that a reimplementation would be possibly
> > > completely refactored to fit into FFmpeg.
> > > 
> > > > (A library
> > > > that's not going to be abandoned like it happened with libdcadec after it
> > > > was merged into lavc,
> > > 
> > > OTOH, this library is quite outside the scope of the FFmpeg, so it
> > > makes sense to keep it as external dependency. This is a quite
> > > different use case than a decoder, a QR-decoder library can make sense
> > > outside of a multimedia library, for a decoder you would need a
> > > complete multimedia library anyway.
> > > 
> > > I was checking the code, and porting would be a serious effort and
> > > comprise several thousands lines of code (against the moderate effort
> > > of wrapping it - which is already done), also some of the logic would
> > > not really fit into FFmpeg because it is quite specific to this
> > > application domain (QR code decoding), so it makes sense for it to
> > > live within an external library. Not to mention that this would be
> > > a duplication of effort.
> > 
> > Image analysis is within FFmpeg's scope, which QR code identification and
> > decoding would be about.
> > 
> > > 
> > > *Unless* someone is willing to port/reimplement the code, but this
> > > should not be a blocker for the wrapper and can be done as a separate
> > > step.
> > 
> > No, i'm not blocking anything. Just stating that ideally this would be a
> > native module.
> 
> I'm not against this of course, but as I already stated this would
> require more effort. It is probably worth it (polynomials and Galois
> fields based error correction) as they would be probably reused in
> other parts of the code. Ditto for the QR encoder.
> 
> But I plan to have this functionality in with the wrappers - this
> should also help with an eventual native port of the features.
> 
> I plan to push this patch in a few days, unless I see comments.
> 
> Thanks for the feedback.

Applied.