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

[Devin Heitmueller]

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'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 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