Re: [FFmpeg-devel] [PATCH] add yuv444p support to mcdeint

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 5745 bytes --]

Hi Ethan

On Fri, May 30, 2025 at 12:29:19AM -0500, Ethan Halsall wrote:
> On 5/29/25 7:58 PM, Michael Niedermayer wrote:
> > On Tue, May 27, 2025 at 01:24:57PM -0500, Ethan Halsall wrote:
> > > Signed-off-by: Ethan Halsall <ethanhalsall11@augustana.edu>
> > > ---
> > >   libavfilter/vf_mcdeint.c | 32 ++++++++++++++++++++++++++++----
> > >   1 file changed, 28 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavfilter/vf_mcdeint.c b/libavfilter/vf_mcdeint.c
> > > index 82048b51d0..11ad19f5cd 100644
> > > --- a/libavfilter/vf_mcdeint.c
> > > +++ b/libavfilter/vf_mcdeint.c
> > > @@ -51,8 +51,10 @@
> > >    #include "libavutil/opt.h"
> > >   #include "libavcodec/avcodec.h"
> > > +#include "libavutil/pixdesc.h"
> > >   #include "avfilter.h"
> > >   #include "filters.h"
> > > +#include "formats.h"
> > >   #include "video.h"
> > >    enum MCDeintMode {
> > > @@ -76,6 +78,7 @@ typedef struct MCDeintContext {
> > >       AVPacket *pkt;
> > >       AVFrame *frame_dec;
> > >       AVCodecContext *enc_ctx;
> > > +    const AVPixFmtDescriptor *pix_fmt_desc;
> > >   } MCDeintContext;
> > >    #define OFFSET(x) offsetof(MCDeintContext, x)
> > > @@ -99,6 +102,17 @@ static const AVOption mcdeint_options[] = {
> > >    AVFILTER_DEFINE_CLASS(mcdeint);
> > >   +static const enum AVPixelFormat pix_fmts[] = {
> > > +    AV_PIX_FMT_YUV420P,
> > > +    AV_PIX_FMT_YUV444P,
> > > +    AV_PIX_FMT_NONE
> > > +};
> > > +
> > 
> > Applying: add yuv444p support to mcdeint
> > error: corrupt patch at line 18
> > 
> > this patch is somehow corrupted, that above is not a valid diff
> > make sure the output of git format-patch is not modified
> > 
> > thx
> > 
> > 
> > [...]
> > 
> > 
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> I followed the instructions on the "Sending patches from email clients"
> section of https://ffmpeg.org/developer.html#Submitting-patches-1. I think
> Thunderbird removed some line breaks in the patch though when I opened the
> .eml file.
> 
> Let's try this one:
> 
> 
> Signed-off-by: Ethan Halsall <ethanhalsall11@augustana.edu>
> ---
>  libavfilter/vf_mcdeint.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/libavfilter/vf_mcdeint.c b/libavfilter/vf_mcdeint.c
> index 82048b51d0..11ad19f5cd 100644
> --- a/libavfilter/vf_mcdeint.c
> +++ b/libavfilter/vf_mcdeint.c
> @@ -51,8 +51,10 @@
> 
>  #include "libavutil/opt.h"
>  #include "libavcodec/avcodec.h"
> +#include "libavutil/pixdesc.h"
>  #include "avfilter.h"
>  #include "filters.h"
> +#include "formats.h"
>  #include "video.h"
> 
>  enum MCDeintMode {
> @@ -76,6 +78,7 @@ typedef struct MCDeintContext {
>      AVPacket *pkt;
>      AVFrame *frame_dec;
>      AVCodecContext *enc_ctx;
> +    const AVPixFmtDescriptor *pix_fmt_desc;
>  } MCDeintContext;
> 
>  #define OFFSET(x) offsetof(MCDeintContext, x)

> @@ -99,6 +102,17 @@ static const AVOption mcdeint_options[] = {
> 
>  AVFILTER_DEFINE_CLASS(mcdeint);
> 
> +static const enum AVPixelFormat pix_fmts[] = {
> +    AV_PIX_FMT_YUV420P,
> +    AV_PIX_FMT_YUV444P,
> +    AV_PIX_FMT_NONE
> +};
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    return ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> +}

why not use FILTER_PIXFMTS ?


> +
>  static int config_props(AVFilterLink *inlink)
>  {
>      AVFilterContext *ctx = inlink->dst;
> @@ -122,13 +136,16 @@ static int config_props(AVFilterLink *inlink)
>      mcdeint->enc_ctx = avcodec_alloc_context3(enc);
>      if (!mcdeint->enc_ctx)
>          return AVERROR(ENOMEM);
> +    mcdeint->pix_fmt_desc = av_pix_fmt_desc_get(inlink->format);
> +    if (!mcdeint->pix_fmt_desc)
> +        return AVERROR(ENOMEM);
>      enc_ctx = mcdeint->enc_ctx;
>      enc_ctx->width  = inlink->w;
>      enc_ctx->height = inlink->h;
>      enc_ctx->time_base = (AVRational){1,25};  // meaningless
>      enc_ctx->gop_size = INT_MAX;
>      enc_ctx->max_b_frames = 0;
> -    enc_ctx->pix_fmt = AV_PIX_FMT_YUV420P;
> +    enc_ctx->pix_fmt = inlink->format;
>      enc_ctx->flags = AV_CODEC_FLAG_QSCALE | AV_CODEC_FLAG_LOW_DELAY |
> AV_CODEC_FLAG_RECON_FRAME;
>      enc_ctx->strict_std_compliance = FF_COMPLIANCE_EXPERIMENTAL;
>      enc_ctx->global_quality = 1;
> @@ -201,8 +218,14 @@ static int filter_frame(AVFilterLink *inlink, AVFrame
> *inpic)
> 
>      for (i = 0; i < 3; i++) {
>          int is_chroma = !!i;
> -        int w = AV_CEIL_RSHIFT(inlink->w, is_chroma);
> -        int h = AV_CEIL_RSHIFT(inlink->h, is_chroma);
> +        int w, h;
> +        if (is_chroma) {
> +            w = inlink->w >> mcdeint->pix_fmt_desc->log2_chroma_w;
> +            h = inlink->h >> mcdeint->pix_fmt_desc->log2_chroma_h;
> +        } else {
> +            w = inlink->w;
> +            h = inlink->h;
> +        }
>          int fils = frame_dec->linesize[i];
>          int srcs = inpic    ->linesize[i];
>          int dsts = outpic   ->linesize[i];

pix_fmt_desc could be a local variable in filter_frame()
which would avoid adding it to the context


thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".