[FFmpeg-devel] avcodec/mpeg12dec: ignore scte20 captions when a53 data is also present

Submitted by Aman Gupta on March 6, 2018, 10:25 p.m.

Details

Message ID 20180306222512.20124-1-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta March 6, 2018, 10:25 p.m.
From: Aman Gupta <aman@tmm1.net>

Some streams include both a53 and scte20 data. Before this commit,
the scte20 data would be used instead of the a53 data.

See https://s3.amazonaws.com/tmm1/ccaptions/scte20plusa53.ts,
which produced garbage captions since 3f1a540204a8c187f77b3805d.
---
 libavcodec/mpeg12dec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer March 7, 2018, 5:49 p.m.
On Tue, Mar 06, 2018 at 02:25:12PM -0800, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> Some streams include both a53 and scte20 data. Before this commit,
> the scte20 data would be used instead of the a53 data.
> 
> See https://s3.amazonaws.com/tmm1/ccaptions/scte20plusa53.ts,
> which produced garbage captions since 3f1a540204a8c187f77b3805d.
> ---
>  libavcodec/mpeg12dec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

why does the combination produce garbage ?
why should not both be exported or it be user selectable?

also turning one off for ever seems problematic for concatenated
sequences. not every sequence would need to contain both i guess

[...]
Devin Heitmueller March 7, 2018, 6:09 p.m.
> On Mar 7, 2018, at 12:49 PM, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> On Tue, Mar 06, 2018 at 02:25:12PM -0800, Aman Gupta wrote:
>> From: Aman Gupta <aman@tmm1.net>
>> 
>> Some streams include both a53 and scte20 data. Before this commit,
>> the scte20 data would be used instead of the a53 data.
>> 
>> See https://s3.amazonaws.com/tmm1/ccaptions/scte20plusa53.ts,
>> which produced garbage captions since 3f1a540204a8c187f77b3805d.
>> ---
>> libavcodec/mpeg12dec.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> why does the combination produce garbage ?
> why should not both be exported or it be user selectable?

I suspect part of the issue is that the SCTE-20 data gets jammed into the a53_caption member.  It may make sense to have it put into it’s own field, so that if both are present the encoder can decide which to insert into the final stream.  But having both of them putting data into the same field is likely to produce indeterministic output.

I’ve got basically the same issue in the decklink input where both EIA-608 and EIA-708 VANC can arrive in the same frame, and will need to either implement a similar change (i.e. prefer 708 if both are present), or be prepared to add a new side_data type for 608 so both can be sent and the downstream can choose which to include in the output.

Devin

> also turning one off for ever seems problematic for concatenated
> sequences. not every sequence would need to contain both I guess

It would certainly be good if it were configurable.  I am certain there are users who will say “I know the X format is broken and I want to always send through the Y format", and that may not correspond to the heuristic implemented in the decoder (in this case, to always prefer 708 data over SCTE-20).

Devin
Aman Gupta March 7, 2018, 6:14 p.m.
On Wed, Mar 7, 2018 at 9:49 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Mar 06, 2018 at 02:25:12PM -0800, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > Some streams include both a53 and scte20 data. Before this commit,
> > the scte20 data would be used instead of the a53 data.
> >
> > See https://s3.amazonaws.com/tmm1/ccaptions/scte20plusa53.ts,
> > which produced garbage captions since 3f1a540204a8c187f77b3805d.
> > ---
> >  libavcodec/mpeg12dec.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
>
> why does the combination produce garbage ?



In my sample, the scte20 data by itself produces garbage captions. I'm not
sure why.


> why should not both be exported or it be user selectable?


From what I've seen in US broadcast television, scte20 is only used on
standard-def content and everything else uses normal a53.

I'm not sure how we would export both since there's only one type of side
data.


>
> also turning one off for ever seems problematic for concatenated
> sequences. not every sequence would need to contain both i guess


Yea that's theoreticaly possible, but I'd rather wait to add support until
someone actually sees it in the wild.

Before I added scte20 support a few months ago no one even noticed it was
missing. It doesn't seem to have wide spread use.

Aman


>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The greatest way to live with honor in this world is to be what we pretend
> to be. -- Socrates
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Devin Heitmueller March 7, 2018, 6:34 p.m.
> From what I've seen in US broadcast television, scte20 is only used on
> standard-def content and everything else uses normal a53.

A53 is definitely the more popular standard, and all that is approved for distribution in digital over the air broadcasts.  SCTE-20 would only be found further up the distribution chain, and perhaps in distribution to some cable boxes (although it’s becoming less and less common that it can be decoded since most of the content is encrypted nowadays).

> 
> I'm not sure how we would export both since there's only one type of side
> data.

We would have to add a new side data type, and encoders would have to be changed to look for both.

>> 
>> also turning one off for ever seems problematic for concatenated
>> sequences. not every sequence would need to contain both I guess

Funny enough, I spent my entire morning debugging some issues with playing concatenated TS streams.  If anyone thinks that’s supposed to be working today in ffmpeg, there’s a ton of work to be done in that area.

> 
> 
> Yea that's theoreticaly possible, but I'd rather wait to add support until
> someone actually sees it in the wild.
> 
> Before I added scte20 support a few months ago no one even noticed it was
> missing. It doesn't seem to have wide spread use.

It’s not really that nobody noticed - it’s that most people in the broadcast space until recently had ruled out the ability to use ffmpeg for production because of it’s lack of good support for ancillary data such as captions.  That situation is improving of course, but it’s not so much that “no one even noticed it was missing”.

If changing the framework to support the extra side data format isn’t viable, then certainly prefering A53 over SCTE-20 would be the right way to go.  I would make it configurable though.

Devin
Aman Gupta March 8, 2018, 7:13 a.m.
On Wed, Mar 7, 2018 at 10:35 AM Devin Heitmueller <
dheitmueller@ltnglobal.com> wrote:

>
> > From what I've seen in US broadcast television, scte20 is only used on
> > standard-def content and everything else uses normal a53.
>
> A53 is definitely the more popular standard, and all that is approved for
> distribution in digital over the air broadcasts.  SCTE-20 would only be
> found further up the distribution chain, and perhaps in distribution to
> some cable boxes (although it’s becoming less and less common that it can
> be decoded since most of the content is encrypted nowadays).


>
> >
> > I'm not sure how we would export both since there's only one type of side
> > data.
>
> We would have to add a new side data type, and encoders would have to be
> changed to look for both.


>
> >>
> >> also turning one off for ever seems problematic for concatenated
> >> sequences. not every sequence would need to contain both I guess
>
> Funny enough, I spent my entire morning debugging some issues with playing
> concatenated TS streams.  If anyone thinks that’s supposed to be working
> today in ffmpeg, there’s a ton of work to be done in that area.


>
> >
> >
> > Yea that's theoreticaly possible, but I'd rather wait to add support
> until
> > someone actually sees it in the wild.
> >
> > Before I added scte20 support a few months ago no one even noticed it was
> > missing. It doesn't seem to have wide spread use.
>
> It’s not really that nobody noticed - it’s that most people in the
> broadcast space until recently had ruled out the ability to use ffmpeg for
> production because of it’s lack of good support for ancillary data such as
> captions.  That situation is improving of course, but it’s not so much that
> “no one even noticed it was missing”.


>
> If changing the framework to support the extra side data format isn’t
> viable, then certainly prefering A53 over SCTE-20 would be the right way to
> go.  I would make it configurable though.


Thanks for the background on SCTE-20. I don't really know much about it.

I'm not opposed to adding new side data, but it doesn't sound like it's
worth it in this particular case. Atleast to me; if someone else wants to
pursue that approach I will happily help review and test any patches.

>
To make my patch configurable, I could change the ignore flag I added into
a new option called parse_scte20: default to "prefer a53" like I have now,
but can be set explicitly to "always" or "never". Wdyt?

Aman


>
> Devin
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
David Engel Jan. 4, 2019, 10:15 p.m.
On Thu, Mar 08, 2018 at 07:13:57AM +0000, Aman Gupta wrote:
> On Wed, Mar 7, 2018 at 10:35 AM Devin Heitmueller <
> dheitmueller at ltnglobal.com> wrote:
> 
> >
> > > From what I've seen in US broadcast television, scte20 is only used on
> > > standard-def content and everything else uses normal a53.
> >
> > A53 is definitely the more popular standard, and all that is approved for
> > distribution in digital over the air broadcasts.  SCTE-20 would only be
> > found further up the distribution chain, and perhaps in distribution to
> > some cable boxes (although it’s becoming less and less common that it can
> > be decoded since most of the content is encrypted nowadays).
> 
> 
> >
> > >
> > > I'm not sure how we would export both since there's only one type of side
> > > data.
> >
> > We would have to add a new side data type, and encoders would have to be
> > changed to look for both.
> 
> 
> >
> > >>
> > >> also turning one off for ever seems problematic for concatenated
> > >> sequences. not every sequence would need to contain both I guess
> >
> > Funny enough, I spent my entire morning debugging some issues with playing
> > concatenated TS streams.  If anyone thinks that’s supposed to be working
> > today in ffmpeg, there’s a ton of work to be done in that area.
> 
> 
> >
> > >
> > >
> > > Yea that's theoreticaly possible, but I'd rather wait to add support
> > until
> > > someone actually sees it in the wild.
> > >
> > > Before I added scte20 support a few months ago no one even noticed it was
> > > missing. It doesn't seem to have wide spread use.
> >
> > It’s not really that nobody noticed - it’s that most people in the
> > broadcast space until recently had ruled out the ability to use ffmpeg for
> > production because of it’s lack of good support for ancillary data such as
> > captions.  That situation is improving of course, but it’s not so much that
> > “no one even noticed it was missing”.
> 
> 
> >
> > If changing the framework to support the extra side data format isn’t
> > viable, then certainly prefering A53 over SCTE-20 would be the right way to
> > go.  I would make it configurable though.
> 
> 
> Thanks for the background on SCTE-20. I don't really know much about it.
> 
> I'm not opposed to adding new side data, but it doesn't sound like it's
> worth it in this particular case. Atleast to me; if someone else wants to
> pursue that approach I will happily help review and test any patches.
> 
> >
> To make my patch configurable, I could change the ignore flag I added into
> a new option called parse_scte20: default to "prefer a53" like I have now,
> but can be set explicitly to "always" or "never". Wdyt?

I tried to use ffmpeg to transcode some TV recordings I have where I
need to keep the closed captions.  When I finally got the ffmpeg
options right to copy the captions, the captions were a garbled,
unrecognizable mess.  I eventually worked around the problem with
ffmpeg be using ccextractor to first extract the captions separately.
I then muxed them back in later with the transcoded video and audio
using ffmpeg.

With ccextractor, I initially got garbled captions too until I used
the --noscte20 option.  After that, I got perfectly clean captions.
When I looked to see if ffmpeg had a comparable switch, I found this
thread.  I build ffmpeg myself with this patch and sure enough I got
perfectly clean captions too without having to use ccextractor.  I see
this patch hasn't been included in ffmpeg yet.  Please reconsider
including it in some form as it is definitely needed out here in the
real world.

Please note that I am not subscribed to the ffmpeg-devel list.  Please
copy me on any replies that you need me to see.

David

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 9e076e89da..45d0a9d737 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -59,6 +59,7 @@  typedef struct Mpeg1Context {
     int has_stereo3d;
     uint8_t *a53_caption;
     int a53_caption_size;
+    int ignore_scte20;
     uint8_t afd;
     int has_afd;
     int slice_count;
@@ -2241,6 +2242,7 @@  static int mpeg_decode_a53_cc(AVCodecContext *avctx,
         /* extract A53 Part 4 CC data */
         int cc_count = p[5] & 0x1f;
         if (cc_count > 0 && buf_size >= 7 + cc_count * 3) {
+            s1->ignore_scte20 = 1;
             av_freep(&s1->a53_caption);
             s1->a53_caption_size = cc_count * 3;
             s1->a53_caption      = av_malloc(s1->a53_caption_size);
@@ -2253,7 +2255,8 @@  static int mpeg_decode_a53_cc(AVCodecContext *avctx,
         }
         return 1;
     } else if (buf_size >= 2 &&
-               p[0] == 0x03 && (p[1]&0x7f) == 0x01) {
+               p[0] == 0x03 && (p[1]&0x7f) == 0x01 &&
+               !s1->ignore_scte20) {
         /* extract SCTE-20 CC data */
         GetBitContext gb;
         int cc_count = 0;