Hi Anton,
Thanks for your feedback. Comments inline:
On Wed, May 3, 2023 at 5:20 AM Anton Khirnov <an...@khirnov.net> wrote:
>
> 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.
Ok.
> > + 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.
Ok.
> > + 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.
Ok.
> > +{
> > + 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.
Yeah, I see your point. Let me investigate further and see what I can do here.
> > + 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.
Ok.
> > +
> > + 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.
So I had previously considered the approach you suggested, but decided
against it. This is because while the AVCCFifo today always returns
the same number of bytes, there are cases where cc_count could vary on
a per frame basis (and thus the buffer size differs). In particular
with 30i with 3:2 pulldown the cc_count alternates between 20 and 30.
See CTA-708-E R-2018 Sec 4.6.3.1.
Having the queue return a properly sized buffer avoids situations
where you make a call to get the size and then make a separate call to
fill the buffer (where the size might not be correct).
I don't particularly love the approach, given it means callers have to
memcpy() the result into the final buffer and then free the buffer
created by the call to inject. But it seemed like the better approach
given the issue described above.
I guess because the API is private and we don't support that feature
today, we could do it as you described and then change the API later.
Suggestions welcome.
> > + 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.
I'll review the logic above based on your comments and if appropriate
rework the loop.
Thanks,
Devin
--
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com e: devin.heitmuel...@ltnglobal.com
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
