[FFmpeg-devel] [PATCH v3 1/5] ccfifo: Properly handle CEA-708 captions through framerate conversion

[Lance Wang]

On Wed, Apr 26, 2023 at 10:14 PM Devin Heitmueller <
devin.heitmueller at ltnglobal.com> wrote:

> Hi Lance,
>
> Thank you for your review.  Comments inline.
>
> On Tue, Apr 25, 2023 at 10:28 AM Lance Wang <lance.lmwang at gmail.com>
> wrote:
> > > +    /* Based on the target FPS, figure out the expected cc_count and
> > > number of
> > > +       608 tuples per packet.  See ANSI/CTA-708-E Sec 4.3.6.1. */
> > > +    for (i = 0; i < (sizeof(cc_lookup_vals) / sizeof(struct
> cc_lookup));
> > > i++) {
> > >
> >
> > I prefer to use FF_ARRAY_ELEMS here.
>
> Ok.
>
> > > +        if (framerate->num == cc_lookup_vals[i].num &&
> > > +            framerate->den == cc_lookup_vals[i].den) {
> > > +            ccf->expected_cc_count = cc_lookup_vals[i].cc_count;
> > > +            ccf->expected_608 = cc_lookup_vals[i].num_608;
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    if (ccf->expected_608 == 0) {
> > > +        av_log(ccf->log_ctx, AV_LOG_WARNING, "cc_fifo cannot transcode
> > > captions fps=%d/%d\n",
> > > +               framerate->num, framerate->den);
> > > +        return NULL;
> > >
> >
> > why not use goto error?  I feel ccf should be freed.
>
> Good point.  I'll fix that.
>
> >
> >
> > > +    }
> > > +
> > > +    return ccf;
> > > +
> > > +error:
> > > +    ff_ccfifo_freep(&ccf);
> > > +    return NULL;
> > > +}
> > > +
> > > +int ff_ccfifo_inject(AVCCFifo *ccf, AVFrame *frame)
> > > +{
> > > +    AVFrameSideData *sd;
> > > +    int cc_filled = 0;
> > > +    int i;
> > > +
> > > +    if (!ccf)
> > > +        return 0;
> > >
> >
> > + * @return            Zero on success, or negative AVERROR
> > + *                    code on failure.
> >
> >  why not return error code?  the same to other failure condition.
>
> Ok, so there are legal cases where ccf is NULL and it isn't an error
> condition.  If the creation of the FIFO fails due to an unsupported
> output framerate, the expectation is that you can continue to call the
> inject/extract functions and they will simply do nothing (i.e. it will
> work in passthrough mode).  There are two alternatives to this
> approach:
>
> 1.  Continue to have the FIFO creation fail (returning a NULL
> pointer), and then have to make sure every caller of extract/inject
> checks for the NULL pointer prior to calling the function.
>
> 2.  Have the FIFO creation report the warning but "succeed" and create
> the FIFO, and then have the inject/extract functions check some flag
> within the ccf structure and do nothing if the flag is set.
>
>
I prefer to 2

I'm open to ideas on the best approach here.
>
> > +
> > > +    if (ccf->cc_detected == 0 || ccf->expected_cc_count == 0)
> > > +        return 0;
> > > +
> > > +    sd = av_frame_new_side_data(frame, AV_FRAME_DATA_A53_CC,
> > > +                                ccf->expected_cc_count *
> > > CC_BYTES_PER_ENTRY);
> > > +    if (!sd)
> > > +        return 0;
> > >
> >
> > same.
>
> Ok.
>
> > > +int ff_ccfifo_extract(AVCCFifo *ccf, AVFrame *frame)
> > > +{
> > > +    int i;
> > > +
> > > +    if (!ccf)
> > > +        return 0;
> > >
> >
> > + * @return            Zero on success, or negative AVERROR
> > + *                    code on failure.
> > same question.
>
> Same explanation as for ff_ccfifo_inject() above,
>
> > > +#ifndef AVUTIL_CCFIFO_H
> > > +#define AVUTIL_CCFIFO_H
> > >
> >
> > AVUTIL is wrong here
>
> Ok.
>
> >
> > > +
> > > +#include "libavutil/avutil.h"
> > > +#include "libavutil/frame.h"
> > > +#include "libavutil/fifo.h"
> > > +
> > > +typedef struct AVCCFifo AVCCFifo;
> > > +
> > > +/**
> > > + * Allocate an AVCCFifo.
> > > + *
> > > + * @param sample_fmt  sample format
> > > + * @param channels    number of channels
> > > + * @param nb_samples  initial allocation size, in samples
> > >
> >
> > This is mismatch comments
>
> Ok.
>
> > > + * @return            newly allocated AVCCFifo, or NULL on error
> > > + */
> > > +AVCCFifo *ff_ccfifo_alloc(AVRational *framerate, void *log_ctx);
> > > +
> > > +/**
> > > + * Free an AVCCFifo
> > > + *
> > > + * @param ccf Pointer to the pointer to the AVCCFifo which should be
> freed
> > > + * @note `*ptr = NULL` is safe and leads to no action.
> > > + */
> > > +void ff_ccfifo_freep(AVCCFifo **ccf);
> > > +
> > > +
> > > +/**
> > > + * Read a frame into a CC Fifo
> > >
> >
> > It's not clear I think.
>
> I don't love the "inject/extract" naming, but I couldn't think of a
> better name (I've actually renamed those functions a couple of times
> over the years I had this code in a non-upstream tree).  In particular
> because the extract function both extracts/removes the bytes from the
> frame and inserts them into the queue, the naming can be a bit
> confusing (and vice versa for the inject function).
>

I only think the comments "Read a frame into a CC fifo" isn't clear for the
function, I'm OK with the function name.



> I welcome suggestions on a better name that more clearly describes
> what the two functions do.
>
> Again, thanks for your comments.  The majority of the issues you
> raised are simple enough to fix, and I welcome suggestions on the
> others.
>
> Devin
>
> --
> Devin Heitmueller, Senior Software Engineer
> LTN Global Communications
> o: +1 (301) 363-1001
> w: https://ltnglobal.com  e: devin.heitmueller at ltnglobal.com
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>