[FFmpeg-devel] BUG in use of extradata and extradata_size with dvb subtitles and teletext

[Andriy Lysnevych]

Hi,

We decided to fix DVB subtitles without touching extradata. Please review
it. The patch does:

1) Improved DVB subtitles encoder to generate AVPacket.data in the same
format as generates MPEGTS demuxer + DVB subtitles parser. So now single
format of DVB subtitles data is used across all the components of FFmpeg:
only subtitles payload WITHOUT 0x20 0x00 bytes at the beginning and 0xFF
trailing byte.

2) Improved MPEGTS muxer to support format of DVB subtitles in
AVPacket.data described above: while muxing we add two bytes 0x20 0x00 to
the beginning of and 0xFF to the end of DVB subtitles payload.

Because of changes described above automatically were fixed few FFmpeg
issues:

3) Because now MPEGTS muxer and demuxer work with the same format of DVB
subtitles, problem described in ticket #2989 were fixed: copy of DVB
subtitles from MPEGTS to MPEGTS stream.

4) Fixed similar DVB subtitles copy operations: Matroska -> MPEGTS, WTV ->
MPEGTS and most likely NUT -> MPEGTS.

5) Fixed muxing of DVB subtitles generated by DVB subtitles encoder into
Matroska (and probably NUT) containers. Muxing was incorrect because
Matroska requires only DVB subtitles payload but DVB subtitles encoder
generated extra 0x00 byte at the beginning and extra 0xFF byte at the end
of the payload.

Our next patch will fix of DVB teletext copy from MPEGTS to MPEGTS. This
patch will touch extradata.

P.S.
We are still waiting for a correct sample with DVB subtitles in NUT
container to ensure in correctness of our patch towards NUT as well.

Regards,
Andriy Lysnevych

On Tue, Dec 31, 2013 at 9:39 PM, Andriy Lysnevych <
andriy.lysnevych at gmail.com> wrote:

> Unfortunately I have no example of multiple DVB subtitles in one stream.
> We have only example of multiple DVB teletext in one stream that is very
> similar in logic to DVB subtitles.
>
> Finally I found .mkv file with DVB subtitles and examined it. Now I see
> that our patch is wrong and generates wrong DVB subtitles payload for
> matroska container.
>
> But event without our patch DVB subtitle encoder generates incorrect DVB
> subtitles payload for matroska. It generates leading 0x00 that is ok for TS
> muxer but Matroska requires DVB subtitles without leading 0x00.
>
> So what I see currently in FFmpeg:
>
> 1) We have two different DVB subtitles AVPacket.data formats internally in
> FFmpeg that is very bad and lead to bugs.
> 2) Copy of DVB subtitles from TS muxer to TS demuxer broken because of
> point 1)
> 3) DVB subtitles encoder produces correct payload for TS muxer. But this
> payload is incorrect for Matroska muxer. Thus DVB subtitles encoder ->
> Matroska muxer chain is broken.
> 4) Muxing DVB teletext is broken in TS muxer because it incorrectly adds
> 0x20 to beginning of DVB teletext payload. This 0x20 should be added to DVB
> subtitles only.
>
> All of this should be fixed in one patch because all 4 points are very
> interconnected.
>
> Also I have questions about extradata:
> If a developer that uses FFmpeg library generates TS stream he need create
> this extradata and add to DVB subtitles stream manually? It means
> developers have to know exact binary format of this extradata?
>
> P.S.
> Can you provide me with correct .nut file that contains DVB subtitles so I
> can inspect it too?
>
>
> On Tue, Dec 31, 2013 at 4:28 PM, JULIAN GARDNER <joolzg at btinternet.com>wrote:
>
>>
>>
>>
>>
>>
>>
>>
>> >________________________________
>> > From: Andriy Lysnevych <andriy.lysnevych at gmail.com>
>> >To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>> >Cc: serhii.marchuk at gmail.com
>> >Sent: Tuesday, 31 December 2013, 10:23
>> >Subject: Re: [FFmpeg-devel] BUG in use of extradata and extradata_size
>> with dvb subtitles and teletext
>> >
>> >
>> >Hi,
>> >
>> >Thanks for your comments.
>> >
>> >1) As I pointed before there are two different AVPacket.data formats of
>> DVB
>> >subtitles in FFmpeg. It is not obvious and causes developers to do bugs.
>> >Because of this copy subtitles from TS demuxer to TS muxer does not work.
>> >
>> >>the reason it was failing in copy was due to the not copying of the ids
>> >from the input stream to the output stream. >Thats all that was needed.
>> >
>> >We investigated the issue. Subtitles in TS stream should go in format
>> "0x20
>> >0x00 [subtitles data] 0xFF". Because TS demuxer generates AVPacket.data
>> in
>> >different format than TS muxer expects on output you get TS stream with
>> >wrong subtitles in format: "0x20 0x00 [subtitles data]" (i.e. without
>> 0xFF
>> >at the end). That is why copy does not work. Making all DVB subtitles
>> >internally to be in one format fixes the issue.
>> >
>>
>> I think the 0x20 .... 0xff fix should be a seperate patch
>>
>> >> In addition there is another issue: as far as I can tell this patch
>> >breaks backward-compatibility.
>> >> So an application that uses its own MPEG-TS demuxer together with our
>> DVB
>> >subtitle decoder will break after this change.
>> >
>> >Yes our goal is to make all DVB subtitles across all FFmpeg components to
>> >be in one format. It is not possible to do without breaking
>> >backward-compatibility. Otherwise it will cause more bugs in future.
>> >
>> >2) Regarding matroska, nut and wtv:
>> >
>> >>I checked: matroska, nut and wtv all support DVB subtitles in FFmpeg.
>> >>So I would expect that your patch misses changes for all 3 of those
>> >>(though I have a suspicion that the exact formats those use differs
>> >>slightly so that at least stream copy from those into MPEG-TS might not
>> >>work well either way).
>> >
>> >As I see the same DVB subtitles parser is used for all matroska, nut, TS
>> >and wtv. The parser expects DVB subtitles to be in format "0x20 0x00
>> >[subtitles data] 0xFF":
>> >
>> >if (buf_size < 2 || buf[0] != 0x20 || buf[1] != 0x00) {
>> >    av_dlog(avctx, "Bad packet header\n");
>> >    return -1;
>> >}
>> >
>> >...
>> >
>> >else if (*p == 0xff)
>> >
>> >...
>> >
>> >From other hand I see that DVB subtitles encoder generates subtitles in
>> >format "0x00 [subtitles data] 0xFF" (without leading 0x20).
>> >
>> >Only MPEG-TS muxer has "hack" that adds leading 0x20 before each DVB
>> >subtitle payload (actually it incorrectly adds 0x20 to teletext also and
>> >that is why muxing teletext is broken too). Nut and matroska do not add
>> >0x20 before payload to DVB subtitles encoder (can't find this in code).
>> As
>> >the result muxing DVB subtitles into matroska and nut is currently
>> broken.
>> >At least data expected by parser and data generated by muxers is
>> different.
>> >Please correct me if I am wrong.
>> >
>> >I can't find documentation how DVB subtitles should be stored in matroska
>> >or nut. Also I can't find real samples to check it. Please share
>> >documentation or samples with us if you have them.
>>
>> >
>>
>> >Our patch fixes subtitle muxing in matroska and nut. As the result all
>> >subtitles will be in format: "0x20 0x00 [subtitles data] 0xFF" even in
>> nut
>> >and matroska.
>> >
>> >About metadata: as a developer I need ability to specify DVB subtitles
>> >parameters for TS muxer via FFmpeg API to correctly generate PMT of TS
>> >stream. Also I need ability to read/copy the same parameters from TS
>> >demuxer.
>> >If metadata is not the place to store these values please advice better
>> way
>> >to do it.
>> >
>>
>>
>> I still think the extradata way is the way we should fix this and also
>> allow multiple values per pid, not limit it to 4 bytes which it currently
>> does.
>>
>> Can you make the stream you have available with multiplte dvb streams?
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-dvbsubtitle.patch
Type: text/x-patch
Size: 4069 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140108/15d3892a/attachment.bin>