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

[Anton Khirnov]

Quoting Devin Heitmueller (2023-04-28 18:37:46)
> +void ff_ccfifo_freep(AVCCFifo **ccf)
> +{
> +    if (ccf && *ccf) {

Don't check for ccf, it makes no sense to call this function with
ccf==NULL, so silently ignoring it can hide bugs.

> +        AVCCFifo *tmp = *ccf;
> +        if (tmp->cc_608_fifo)
> +            av_fifo_freep2(&tmp->cc_608_fifo);

Useless checks, av_fifo_freep2 can be called on &NULL.

> +        if (tmp->cc_708_fifo)
> +            av_fifo_freep2(&tmp->cc_708_fifo);
> +        av_freep(*ccf);
> +    }
> +}
> +
> +AVCCFifo *ff_ccfifo_alloc(AVRational *framerate, void *log_ctx)

We typically pass AVRational directly, not as pointers. That also makes
it clear that the passed value is not modified.

> +{
> +    AVCCFifo *ccf;
> +    int i;
> +
> +    ccf = av_mallocz(sizeof(*ccf));
> +    if (!ccf)
> +        return NULL;
> +
> +    if (!(ccf->cc_708_fifo = av_fifo_alloc2(MAX_CC_ELEMENTS, CC_BYTES_PER_ENTRY, 0)))
> +        goto error;
> +
> +    if (!(ccf->cc_608_fifo = av_fifo_alloc2(MAX_CC_ELEMENTS, CC_BYTES_PER_ENTRY, 0)))
> +        goto error;
> +
> +    /* 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 < FF_ARRAY_ELEMS(cc_lookup_vals); i++) {
> +        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) {
> +        /* We didn't find an output frame we support.  We'll let the call succeed
> +           and the FIFO to be allocated, but the extract/inject functions will simply
> +           leave everything the way it is */
> +        av_log(ccf->log_ctx, AV_LOG_WARNING, "cc_fifo cannot transcode captions fps=%d/%d\n",
> +               framerate->num, framerate->den);

Won't this result in spurious warnings for users who are just converting
framerates and don't have any captions. If so it should probably be
printed the first time we actually see caption side data.

> +        ccf->passthrough = 1;
> +    }
> +
> +    return ccf;
> +
> +error:
> +    ff_ccfifo_freep(&ccf);
> +    return NULL;
> +}
> +
> +int ff_ccfifo_injectbytes(AVCCFifo *ccf, uint8_t **data, size_t *len)
> +{
> +    char *cc_data;
> +    int cc_filled = 0;
> +    int i;
> +
> +    if (!ccf)
> +        return AVERROR(EINVAL);

For all the extract/inject functions: it should be invalid to call them
with a NULL context, so this should be an av_assert0() or not be here at
all.

> +
> +    if (ccf->passthrough) {
> +        *data = NULL;
> +        *len = 0;
> +        return 0;
> +    }
> +
> +    cc_data = av_mallocz(ccf->expected_cc_count * CC_BYTES_PER_ENTRY);

This buffer size is constant for a given AVCCFifo object, so perhaps
ff_ccfifo_alloc() could return required buffer size and this function
could write into a user-provided buffer and avoid constant dynamic
allocation.

> +    if (!cc_data) {
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    for (i = 0; i < ccf->expected_608; i++) {
> +        if (av_fifo_can_read(ccf->cc_608_fifo) >= CC_BYTES_PER_ENTRY) {
> +            av_fifo_read(ccf->cc_608_fifo, &cc_data[cc_filled * CC_BYTES_PER_ENTRY],
> +                         CC_BYTES_PER_ENTRY);

This looks wrong, as fifo operations are in elements, and your
elements are already CC_BYTES_PER_ENTRY. So every read actually writes
CC_BYTES_PER_ENTRY * CC_BYTES_PER_ENTRY bytes to cc_data.

I think you can also do this as a single av_fifo_read() call without a
loop.

-- 
Anton Khirnov