From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id C00614550B for ; Wed, 3 May 2023 09:20:31 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id AED1568B7F1; Wed, 3 May 2023 12:20:27 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 07D43680303 for ; Wed, 3 May 2023 12:20:21 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id 8CD142404EC; Wed, 3 May 2023 11:20:20 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id rUQapfebBsnt; Wed, 3 May 2023 11:20:19 +0200 (CEST) Received: from lain.khirnov.net (lain.khirnov.net [IPv6:2001:67c:1138:4306::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "lain.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id CB162240177; Wed, 3 May 2023 11:20:19 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id A0B811601B2; Wed, 3 May 2023 11:20:19 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: <1682699871-22331-2-git-send-email-dheitmueller@ltnglobal.com> References: <1682699871-22331-1-git-send-email-dheitmueller@ltnglobal.com> <1682699871-22331-2-git-send-email-dheitmueller@ltnglobal.com> Mail-Followup-To: FFmpeg development discussions and patches , Devin Heitmueller Date: Wed, 03 May 2023 11:20:19 +0200 Message-ID: <168310561962.3843.17840126335542048705@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH v4 1/6] ccfifo: Properly handle CEA-708 captions through framerate conversion X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Devin Heitmueller Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".