[FFmpeg-devel] lavf/mxfdec: Export EIA608 Closed Captions by default

Submitted by Carl Eugen Hoyos on Feb. 16, 2019, 7:31 p.m.

Details

Message ID CAB0OVGpAdfBgbC0A=zdjEZdRv_XphT=jMsJHp4_1cb7GiqnCWw@mail.gmail.com
State New
Headers show

Commit Message

Carl Eugen Hoyos Feb. 16, 2019, 7:31 p.m.
Hi!

I am not sure why there is an option to disable Closed Captions export,
but disabling the export by default seems like a bad idea to me.

Please comment, Carl Eugen

Comments

Marton Balint Feb. 17, 2019, 12:40 a.m.
On Sat, 16 Feb 2019, Carl Eugen Hoyos wrote:

> Hi!
>
> I am not sure why there is an option to disable Closed Captions export,
> but disabling the export by default seems like a bad idea to me.

SMPTE 436M can be any kind of ancillary data, not only closed captions. 
That is why the default behaviour (pass all the data to userspace so a 
userspace app can parse it properly) makes sense. Some applications might 
depend on this.

The eia608_extract option is a hack which effectively replaces the generic 
ancillary data stream with the parsed subtitle stream. The reason for the 
hack is that libavcodec has no API for such a parser or filter which reads 
a data stream and generates one (ore more) subtitle streams...

Regards,
Marton
Carl Eugen Hoyos Feb. 17, 2019, 2:24 a.m.
2019-02-17 1:40 GMT+01:00, Marton Balint <cus@passwd.hu>:
>
> On Sat, 16 Feb 2019, Carl Eugen Hoyos wrote:
>>
>> I am not sure why there is an option to disable Closed Captions export,
>> but disabling the export by default seems like a bad idea to me.
>
> SMPTE 436M can be any kind of ancillary data, not only closed captions.
> That is why the default behaviour (pass all the data to userspace so a
> userspace app can parse it properly) makes sense.

> Some applications might depend on this.

Wouldn't it still make more sense to change the default?
The application that knows it needs the ancillary data, well, knows
it while the average user who reads the mxf file has no idea that
there is a subtitle stream and has no idea that FFmpeg can read
the subtitle stream.

I would bump micro in any case and the option also works fine
with older FFmpeg versions so I don't really see the issue.

Carl Eugen
Marton Balint Feb. 17, 2019, 2:49 p.m.
On Sun, 17 Feb 2019, Carl Eugen Hoyos wrote:

> 2019-02-17 1:40 GMT+01:00, Marton Balint <cus@passwd.hu>:
>>
>> On Sat, 16 Feb 2019, Carl Eugen Hoyos wrote:
>>>
>>> I am not sure why there is an option to disable Closed Captions export,
>>> but disabling the export by default seems like a bad idea to me.
>>
>> SMPTE 436M can be any kind of ancillary data, not only closed captions.
>> That is why the default behaviour (pass all the data to userspace so a
>> userspace app can parse it properly) makes sense.
>
>> Some applications might depend on this.
>
> Wouldn't it still make more sense to change the default?

Sorry, no. We should not change existing behaviour for this.

> The application that knows it needs the ancillary data, well, knows
> it while the average user who reads the mxf file has no idea that
> there is a subtitle stream and has no idea that FFmpeg can read
> the subtitle stream.
>
> I would bump micro in any case and the option also works fine
> with older FFmpeg versions so I don't really see the issue.

Issue is that you are removing a data stream which was previously 
provided. Also CC extraction in MXF decoder is a hack. It should be 
deprecated after the API will be capable of bitstream filtering between 
different codecs. And the VANC data can be parsed to multiple 
subtitle/data streams. Why eia608 has the perference? Why not teletext, or 
SCTE messages? All in all, it is better to keep hacks under the 
option until we come up with better API.

Regards,
Marton
Marton Balint Feb. 17, 2019, 3:28 p.m.
On Sun, 17 Feb 2019, Marton Balint wrote:

>
>
> On Sun, 17 Feb 2019, Carl Eugen Hoyos wrote:
>
>> 2019-02-17 1:40 GMT+01:00, Marton Balint <cus@passwd.hu>:
>>>
>>> On Sat, 16 Feb 2019, Carl Eugen Hoyos wrote:
>>>>
>>>> I am not sure why there is an option to disable Closed Captions export,
>>>> but disabling the export by default seems like a bad idea to me.
>>>
>>> SMPTE 436M can be any kind of ancillary data, not only closed captions.
>>> That is why the default behaviour (pass all the data to userspace so a
>>> userspace app can parse it properly) makes sense.
>>
>>> Some applications might depend on this.
>>
>> Wouldn't it still make more sense to change the default?
>
> Sorry, no. We should not change existing behaviour for this.
>
>> The application that knows it needs the ancillary data, well, knows
>> it while the average user who reads the mxf file has no idea that
>> there is a subtitle stream and has no idea that FFmpeg can read
>> the subtitle stream.
>>
>> I would bump micro in any case and the option also works fine
>> with older FFmpeg versions so I don't really see the issue.
>
> Issue is that you are removing a data stream which was previously 
> provided. Also CC extraction in MXF decoder is a hack. It should be 
> deprecated after the API will be capable of bitstream filtering between 
> different codecs. And the VANC data can be parsed to multiple 
> subtitle/data streams. Why eia608 has the perference? Why not teletext, or 
> SCTE messages? All in all, it is better to keep hacks under the 
> option until we come up with better API.

One more thing is that the fact that a file has VANC data does not mean 
that it actually has Closed Captions data. It might, or it might not, so 
unconditionally advertising a CC stream just because there _might_ be CC 
data would also be bad.

Regards,
Marton
Tomas Härdin Feb. 18, 2019, 8:23 a.m.
sön 2019-02-17 klockan 16:28 +0100 skrev Marton Balint:
> 
> On Sun, 17 Feb 2019, Marton Balint wrote:
> 
> > 
> > 
> > On Sun, 17 Feb 2019, Carl Eugen Hoyos wrote:
> > 
> > > > > > 2019-02-17 1:40 GMT+01:00, Marton Balint <cus@passwd.hu>:
> > > > 
> > > > On Sat, 16 Feb 2019, Carl Eugen Hoyos wrote:
> > > > > 
> > > > > I am not sure why there is an option to disable Closed Captions export,
> > > > > but disabling the export by default seems like a bad idea to me.
> > > > 
> > > > SMPTE 436M can be any kind of ancillary data, not only closed captions.
> > > > That is why the default behaviour (pass all the data to userspace so a
> > > > userspace app can parse it properly) makes sense.
> > > > Some applications might depend on this.
> > > 
> > > Wouldn't it still make more sense to change the default?
> > 
> > Sorry, no. We should not change existing behaviour for this.
> > 
> > > The application that knows it needs the ancillary data, well, knows
> > > it while the average user who reads the mxf file has no idea that
> > > there is a subtitle stream and has no idea that FFmpeg can read
> > > the subtitle stream.
> > > 
> > > I would bump micro in any case and the option also works fine
> > > with older FFmpeg versions so I don't really see the issue.
> > 
> > Issue is that you are removing a data stream which was previously 
> > provided. Also CC extraction in MXF decoder is a hack. It should be 
> > deprecated after the API will be capable of bitstream filtering between 
> > different codecs. And the VANC data can be parsed to multiple 
> > subtitle/data streams. Why eia608 has the perference? Why not teletext, or 
> > SCTE messages? All in all, it is better to keep hacks under the 
> > option until we come up with better API.
> 
> One more thing is that the fact that a file has VANC data does not mean 
> that it actually has Closed Captions data. It might, or it might not, so 
> unconditionally advertising a CC stream just because there _might_ be CC 
> data would also be bad.

Isn't this stuff also used for teletext and whatnot? I generally agree
that we shouldn't assume it's subtitles

/Tomas

Patch hide | download patch | download mbox

From 9ee6b52c22ae516f9f10116381123595ea51bb0a Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Sat, 16 Feb 2019 20:24:03 +0100
Subject: [PATCH] lavf/mxfdec: Extract EIA608 Closed Captions by default.

Related to ticket #5362.
---
 libavformat/mxfdec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 0553adc..b6c9e60 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -3712,7 +3712,7 @@  static int mxf_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
 
 static const AVOption options[] = {
     { "eia608_extract", "extract eia 608 captions from s436m track",
-      offsetof(MXFContext, eia608_extract), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1,
+      offsetof(MXFContext, eia608_extract), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1,
       AV_OPT_FLAG_DECODING_PARAM },
     { NULL },
 };
-- 
1.7.10.4