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

[Marton Balint]

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

>
> 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.

Regards,
Marton