[FFmpeg-devel] [PATCH v2] libavformat/mpegts: demux DVB VBI data

[Scott Theisen]

On 12/8/24 17:04, Marton Balint wrote:
> On Sun, 8 Dec 2024, Scott Theisen wrote:
>> On 12/3/24 17:23, Marton Balint wrote:
>>>   On Tue, 3 Dec 2024, Marton Balint wrote:
>>>>   On Sat, 30 Nov 2024, Scott Theisen wrote:
>>>>>   DVB VBI data is defined in ETSI EN 301 775 and can include EBU 
>>>>> teletext
>>>>>   data
>>>>>   as defined in ETSI EN 300 472.
>>>>>
>>>>>   ETSI EN 300 468 defines teletext_descriptor, 
>>>>> VBI_data_descriptor, and
>>>>>   VBI_teletext_descriptor, which has the same definition as, but
>>>>>  different
>>>>>   use
>>>>>   from, teletext_descriptor.
>>>>>   ---
>>>>>   libavcodec/codec_desc.c | 6 ++++++
>>>>>   libavcodec/codec_id.h   | 1 +
>>>>>   libavformat/mpegts.c    | 3 +++
>>>>>   libavformat/mpegts.h    | 2 ++
>>>>>   4 files changed, 12 insertions(+)
>>>>
>>>>  You should split this to two patches.
>>
>> OK, I'll do that.
>>
>>>>
>>>>  Patch 1 should add the codec ID the codec_description and please also
>>>>  update the assertion check in libavcodec/version.c.
>>>>
>>>>  Patch 2 should add the support for demuxing in mpegts. I'd rather 
>>>> put the
>>>>  VBI descriptors after the teletext descriptor in the list, so in case
>>>>  multiple descriptors are present the detected codec should be 
>>>> teletext.
>>>
>>>  On second thought the order does not help, because the codec will be
>>>  determined by the first descriptor...
>>>
>>>  Maybe when parsing the teletext descriptor it should override VBI 
>>> codec to
>>>  teletext, e.g.:
>>>
>>>      case TELETEXT_DESCRIPTOR:
>>>           if (codec_id == DVB_VBI)
>>>                  codec_id = DVB_TELETEXT
>>>           // fall-through
>>>      case VBI_TELETEXT_DESCRIPTOR:
>>>           ....
>>
>> I'm not sure it makes sense to change it from DVB_VBI to 
>> DVB_TELETEXT, since the VBI format is backwards compatible with the 
>> teletext format.  Although, the TELETEXT_DESCRIPTOR is for EBU 
>> teletext data only streams, but then there shouldn't be either VBI 
>> descriptor to set the codec_id to DVB_VBI.
>>
>> I'm not strongly opposed, I'm just not sure it is really necessary.
>
> The reason I prefer it that way so streams which were detected earlier 
> as teletext won't be suddenly detected as VBI because both descriptors 
> are present and VBI is the first. I think using both descriptors is 
> not unlikely, since the essence is compatible, here is an example I 
> stumbled upon by accident:
>
> https://www.dvbcontrol.com/wp-content/uploads/DVBAnalyzer/DVBAnalyzer_View_0.jpg 
>

ETSI EN 300 468 is not very clear on how the descriptors are supposed to 
be used, but I think it goes like this:

"The VBI_data_descriptor (see table 106) shall be used in the PSI PMT of 
a stream which carries VBI data as defined in ETSI EN 301 775."

However, a stream as defined by ETSI EN 300 472 (teletext) is also a 
stream as defined by ETSI EN 301 775 (VBI data), so a stream that is 
just EBU teletext shall have a teletext_descriptor and a 
VBI_data_descriptor.

I think a VBI_teletext_descriptor would be used instead of a 
teletext_descriptor when there are other types of VBI data in addition 
to the EBU teletext data, but it is not clear if they are supposed to be 
mutually exclusive.

>
>>
>> If you do want it, I'm not sure the if is necessary, just set 
>> codec_id unconditionally.  However, it might be better to just call 
>> mpegts_find_stream_type() instead so FFStream::need_context_update is 
>> set.
>
> Yes, good point calling mpegts_find_stream_type() instead.
>
>>
>> Regardless, I'm wondering if instead VBI_TELETEXT_DESCRIPTOR should 
>> not be added to DESC_types since ETSI EN 300 468 says this about 
>> VBI_teletext_descriptor:
>> "
>> The only exception is that the VBI_teletext_descriptor is not to be 
>> used to associate stream_type 0x06 with the VBI standard nor the EBU 
>> teletext standard. Decoders can only use the languages in this 
>> descriptor to select magazines and subtitles.
>> "
>>
>
> Yeah, we could remove it from DESC_types if we want to strictly follow 
> the standard. However, as far as understand, a VBI_teletext_descriptor 
> should never appear alone, only accompanied by a VBI_data_descriptor. 
> So the question is if there are non-compliant streams with 
> VBI_teletext_descriptor only which we want to support? I don't really 
> have a preference here, so do whichever you think best.

I don't see any harm in keeping it in DESC_types to support possible 
non-compliant streams.

All of this discussion has made me think that DVB_VBI shouldn't be a 
separate codec, but added to DVB_TELETEXT, changing the 
AVCodecDescriptor::long_name to "DVB teletext and VBI data", since the 
data format is the same.  The only downside I see is that a VBI data 
stream without any EBU teletext data would appear to be a subtitle 
stream even though it contains no subtitles.  However, that issue is 
also present with the v2 patch which says DVB_VBI is subtitles.

It would be nice, although not necessary, if the information in the 
VBI_data_descriptor was exposed somehow, but the teletext descriptors 
already use AVCodecParameters::extradata and I'm not sure if 
AVStream::metadata is supposed to be only text data, so I don't know 
where the entire VBI_data_descriptor could be copied to.

Regards,

Scott Theisen