* Re: [FFmpeg-devel] [PATCH 1/3] closed caption decoder: accept and decode a new codec type of 'raw 608 byte pairs'
2023-06-20 0:50 ` [FFmpeg-devel] [PATCH 1/3] closed caption decoder: accept and decode a new codec type of 'raw 608 byte pairs' Roger Pack
@ 2023-06-20 7:08 ` Paul B Mahol
2023-06-20 14:34 ` Devin Heitmueller
1 sibling, 0 replies; 3+ messages in thread
From: Paul B Mahol @ 2023-06-20 7:08 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Tue, Jun 20, 2023 at 2:50 AM Roger Pack <rogerdpack2@gmail.com> wrote:
> OK updated patches to work with latest git master, made coded id less
> verbose.
> These have been tested against a "real device" and seem to work properly.
> I'd like to get feedback on the Closed caption decoder first if that's
> possible.
> I pinged the decoder "maintainer" about it once and didn't get a
> response so seems it's up to us.
>
>
Please remove '_dec_' part, its really not needed.
Thank you.
> -=roger=-
>
> On Thu, May 7, 2020 at 7:06 AM Paul B Mahol <onemda@gmail.com> wrote:
> >
> > Is it just me, or the coded id is too much verbose?
> >
> > On 5/7/20, Anton Khirnov <anton@khirnov.net> wrote:
> > > Quoting Roger Pack (2020-04-28 08:15:19)
> > >> From 5d7c12a3f703e794e1092087355bc9523d5f4d93 Mon Sep 17 00:00:00 2001
> > >> From: rogerdpack <rogerpack2005@gmail.com>
> > >> Date: Tue, 28 Apr 2020 05:15:15 +0000
> > >> Subject: [PATCH 1/3] closed caption decoder: accept and decode a new
> codec
> > >> type of 'raw 608 byte pairs'
> > >>
> > >> Signed-off-by: rogerdpack <rogerpack2005@gmail.com>
> > >> ---
> > >> diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h
> > >> index e7d6e059db..805e18758b 100644
> > >> --- a/libavcodec/codec_id.h
> > >> +++ b/libavcodec/codec_id.h
> > >> @@ -513,6 +513,7 @@ enum AVCodecID {
> > >>
> > >> AV_CODEC_ID_MICRODVD = 0x17800,
> > >> AV_CODEC_ID_EIA_608,
> > >> + AV_CODEC_ID_EIA_608_RAW_BYTE_PAIRS,
> > >
> > > You can't just add new IDs in the middle of the table, it changes the
> > > IDs of all the following codecs which breaks ABI.
> > > Add it to the end of the subtitle block.
> > >
> > > --
> > > 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".
> > _______________________________________________
> > 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".
> _______________________________________________
> 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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [FFmpeg-devel] [PATCH 1/3] closed caption decoder: accept and decode a new codec type of 'raw 608 byte pairs'
2023-06-20 0:50 ` [FFmpeg-devel] [PATCH 1/3] closed caption decoder: accept and decode a new codec type of 'raw 608 byte pairs' Roger Pack
2023-06-20 7:08 ` Paul B Mahol
@ 2023-06-20 14:34 ` Devin Heitmueller
1 sibling, 0 replies; 3+ messages in thread
From: Devin Heitmueller @ 2023-06-20 14:34 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Hi Roger,
On Mon, Jun 19, 2023 at 8:50 PM Roger Pack <rogerdpack2@gmail.com> wrote:
>
> OK updated patches to work with latest git master, made coded id less verbose.
> These have been tested against a "real device" and seem to work properly.
> I'd like to get feedback on the Closed caption decoder first if that's possible.
> I pinged the decoder "maintainer" about it once and didn't get a
> response so seems it's up to us.
>
> Thank you.
> -=roger=-
First off, to be clear, I'm generally supportive of your efforts to
extend the DirectShow interface to support delivery of 608 captions.
That said, I have some concerns about the specific approach you've
taken, and in particular the notion of introducing a new codec type.
It's been a while since I've looked at the BDA interface for the VBI
slicer, but IIRC there are two CC output pins, and the first one
delivers the sliced byte pairs for CC1/CC2 and the second pin delivers
CC3/CC4. Your patch to the Directshow driver though only operates on
the first pin, so any captions on CC3/CC4 would be lost.
Further, your new codec provides no way to know whether the packets
containing the byte pairs are for CC1/CC2 or CC3/CC4. We need to be
able to distinguish between the two, since knowing which service is
carrying the captions is important and some data types (e.g. XDS) are
only delivered on certain services. The original AV_CODEC_ID_EIA_608
codec allows downstream components processing the packets to know
which pair it is based on the prefix byte.
My inclination is that adding a new codec type creates additional
maintenance headaches, as various components needs to be modified to
handle both codecs, and in fact the *only* difference between your
codec and the existing one is the presence of the prefix byte (which
is actually needed in order to tell which pair it is). So why not
simply have the directshow component add the 0xFC byte to the front of
the payload for CC1/CC2 and 0xFD for CC3/CC4? Doing this would allow
everything else to stay the same and no additional codec would be
required? It would also allow you to add support for CC3/CC4 after
wiring it up to the other CC output pin on the BDA VBI slicer?
In other words, it seems like a 1-line change to the dshow patch would
eliminate the need for the first patch completely.
On a side-note, it's probably worth noting that using a three-byte
payload predates CEA-708. The prefix byte was present in earlier
standards such as use in DVD GOPs, although the actual structure of
the prefix byte differs in various standards. While the format for
the ffmpeg codec does match what's found in CEA-708, that's mostly
just for the sake of consistency.
Devin
--
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com e: devin.heitmueller@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 3+ messages in thread