On Wed, Apr 26, 2023 at 10:14 PM Devin Heitmueller < devin.heitmuel...@ltnglobal.com> wrote:
> Hi Lance, > > Thank you for your review. Comments inline. > > On Tue, Apr 25, 2023 at 10:28 AM Lance Wang <lance.lmw...@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.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". > _______________________________________________ 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".