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

[Marton Balint]

On Tue, 10 Dec 2024, Scott Theisen wrote:

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

Yeah, this is also how I understand it should work.

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

I'd rather not change DVB_TELETEXT codec bitstream format to the extended 
VBI format. Changing codec bitstream formats can be considered a breaking 
change and it can also cause problems, like all of a sudden remuxing 
DVB_TELETEXT would require a different descriptor depending on 
packet contents... So having a DVB_VBI as a separate codec still looks 
better to me.

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

I guess it could be exported as AVCodecParameters->coded_side_data.

Regards,
Marton