[FFmpeg-devel] [PATCH] DVB Subtitles Fix
Ivan Kalvachev
ikalvachev at gmail.com
Fri May 6 16:28:24 CEST 2011
On 5/6/11, JULIAN GARDNER <joolzg at btinternet.com> wrote:
>
>
>
>>________________________________
>>From: Ivan Kalvachev <ikalvachev at gmail.com>
>>To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>>Sent: Friday, 6 May 2011, 12:56
>>Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>
>>On 5/5/11, JULIAN GARDNER <joolzg at btinternet.com> wrote:
>>> ----- Original Message -----
>>>> From: Ivan Kalvachev <ikalvachev at gmail.com>
>>>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>>>> Cc:
>>>> Sent: Wednesday, 4 May 2011, 21:32
>>>> Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>>>
>>>> On 5/4/11, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>>> On Wed, May 04, 2011 at 07:18:12PM +0100, JULIAN GARDNER wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ----- Original Message -----
>>>>>> > From: Michael Niedermayer <michaelni at gmx.at>
>>>>>> > To: FFmpeg development discussions and patches
>>>> <ffmpeg-devel at ffmpeg.org>
>>>>>> > Cc:
>>>>>> > Sent: Wednesday, 4 May 2011, 18:25
>>>>>> > Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>>>>> >
>>>>>> > On Wed, May 04, 2011 at 04:11:33PM +0100, JULIAN GARDNER wrote:
>>>>>> >>
>>>>>> >>
>>>>>> >>
>>>>>> >>
>>>>>> >> ----- Original Message -----
>>>>>> >> > From: Michael Niedermayer <michaelni at gmx.at>
>>>>>> >> > To: FFmpeg development discussions and patches
>>>>>> > <ffmpeg-devel at ffmpeg.org>
>>>>>> >> > Cc:
>>>>>> >> > Sent: Wednesday, 4 May 2011, 15:14
>>>>>> >> > Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>>>>> >> >
>>>>>> >> > On Tue, Apr 26, 2011 at 11:34:10PM +0100, JULIAN GARDNER
>>>> wrote:
>>>>>> >> >> Ok first patch to fix the decoding and re-encoding
>>>> of DVB
>>>>>> > Subtitles.
>>>>>> >> >>
>>>>>> >> >> I went through and validated against the spec, my
>>>> own engine and
>>>>>> > fixed the
>>>>>> >> > bits that were failing or wrong.
>>>>>> >> >
>>>>>> >> > please explain how to reproduce the bugs that your patch
>>>> fixes
>>>>>> >> >
>>>>>> >> > Also please explain more elaborately what and why each
>>>> change is
>>>>>> >> made.
>>>>>> >> >
>>>>>> >>
>>>>>> >> I use LIVE and RECORDED streams from various sources which i
>>>> have
>>>>>> >> collected
>>>>>> > over the past years, these are put through ffmpeg and then the
>>>> output
>>>>>> > stream is
>>>>>> > played out in VLC and a couple of my own receivers which are used
>>>> in
>>>>>> > professional cable networks.
>>>>>> >
>>>>>> > understood, but does this patch fix any problem with these ffmpeg
>>>>>> > generated streams?
>>>>>> > Iam asking because id like to reproduce the problem and the fix.
>>>>>> >
>>>>>> >
>>>>>> it fixes all the problems with the encoded streams, also it allows
>>>> FFMPEG
>>>>>> to process a file correctly without bailing out with an error.
>>>>>>
>>>>>> I can upload a couple of streams if you want for you to see the
>>>> problems.
>>>>>
>>>>> thanks, yes this would help me.
>>>>>
>>>>> if they are small you can upload to our issue tracker
>>>>> http://ffmpeg.org/trac/ffmpeg
>>>>>
>>>>>
>>>>>>
>>>>>> >>
>>>>>> >> >
>>>>>> >> >>
>>>>>> >> >> I have tested this on recorded streams and live
>>>> streams and found
>>>>>> > no
>>>>>> >> > problems except for one which i will try and explain
>>>> about.
>>>>>> >> >>
>>>>>> >> >> Here in the UK some of our channels use a live
>>>> speech convertor
>>>>>> > where you
>>>>>> >> > see the words appear as they are spoken, now this works
>>>> fine but the
>>>>>> >>
>>>>>> > way the
>>>>>> >> > decode/encode works a lot more space is taken by this
>>>> encoder for
>>>>>> >> the
>>>>>> > subsequent
>>>>>> >> > encoded stream. In one test the incoming packets made up
>>>> around 3k
>>>>>> >> of
>>>>>> > data, but
>>>>>> >> > the amount of data in the output stream was around 15k.
>>>>>> >> >>
>>>>>> >> >> joolz
>>>>>> >> >
>>>>>> >> >> ffmpeg.c | 11 -
>>>>>> >> >> libavcodec/dvbsub.c | 103 ++++++++---------
>>>>>> >> >> libavcodec/dvbsubdec.c | 296
>>>>>> >> > +++++++++++++++++++++++++++++--------------------
>>>>>> >> >> libavcodec/utils.c | 3
>>>>>> >> >> 4 files changed, 233 insertions(+), 180
>>>> deletions(-)
>>>>>> >> >> d22a3a04cf764a0b25138652ad82fe484311405f
>>>> dvbsubs.diff
>>>>>> >> >> diff --git a/ffmpeg.c b/ffmpeg.c
>>>>>> >> >> index 2c69608..e88bcfc 100644
>>>>>> >> >> --- a/ffmpeg.c
>>>>>> >> >> +++ b/ffmpeg.c
>>>>>> >> >> @@ -1092,15 +1092,8 @@ static void
>>>>>> > do_subtitle_out(AVFormatContext *s,
>>>>>> >> >> subtitle_out =
>>>> av_malloc(subtitle_out_max_size);
>>>>>> >> >> }
>>>>>> >> >>
>>>>>> >> >> - /* Note: DVB subtitle need one packet to draw
>>>> them and one
>>>>>> > other
>>>>>> >> >> - packet to clear them */
>>>>>> >> >> - /* XXX: signal it in the codec context ? */
>>>>>> >> >> - if (enc->codec_id == CODEC_ID_DVB_SUBTITLE)
>>>>>> >> >> - nb = 2;
>>>>>> >> >> - else
>>>>>> >> >> - nb = 1;
>>>>>> >> >> -
>>>>>> >> >> - for(i = 0; i < nb; i++) {
>>>>>> >> >> + i = 0;
>>>>>> >> >> + {
>>>>>> >> >> sub->pts = av_rescale_q(pts,
>>>>>> > ist->st->time_base,
>>>>>> >> > AV_TIME_BASE_Q);
>>>>>> >> >> // start_display_time is required to be 0
>>>>>> >> >> sub->pts +=
>>>>>> >> > av_rescale_q(sub->start_display_time, (AVRational){1,
>>>> 1000},
>>>>>> > AV_TIME_BASE_Q);
>>>>>> >> >
>>>>>> >> > why is this changed?
>>>>>> >> >
>>>>>> >>
>>>>>> >> Because only one stream is needed to process DVB Subtitles, i
>>>> could
>>>>>> >> not
>>>>>> > work out why you needed this. The NOTE is wrong regarding needed
>>>> one to
>>>>>> > draw and
>>>>>> > one to remove
>>>>>> >>
>>>>>> >> What you will get from the DVB Stream is a packet with NO
>>>> regions,
>>>>>> >> this is
>>>>>> > the one that removes the subtitles from the picture, as per the
>>>> spec,
>>>>>> > there is a
>>>>>> > timeout value, which in the ffmpeg engine is always set to 30
>>>> seconds,
>>>>>> > this
>>>>>> > would cause the receiving unit to switch off the display after 30
>>>>>> > seconds of no
>>>>>> > valid dvb stream.
>>>>>> >
>>>>>> > Lets assume something generates subtitles, lets say it generates
>>>> one
>>>>>> > displaying "hello world" at start time 0:10 and end time
>>>> 0:50
>>>>>> > If i understand you correctly this needs a packet muxed with time
>>>> 0:10
>>>>>> > and a empty clear packet muxed at time 0:50
>>>>>> >
>>>>>> > And this loop generates these 2 packets from 1 subtitle with start
>>>> and
>>>>>> > end times.
>>>>>> >
>>>>>> > You remove it and i dont see how this could still work afterwards
>>>>>> > maybe iam missing something ?
>>>>>>
>>>>>> The subtitles dont have an end time per se, what you have is a
>>>>>> display
>>>>>> time and a timeout value, this is incase of a stream problem or
>>>> corruption
>>>>
>>>>>> for a missing packet.
>>>>>>
>>>>>> now what happens is in you example you would get a display packet
>>>>>> with
>>>>>> regions/objects/cluts etc and a timeout of 60, then around 50 seconds
>>>> into
>>>>>> the stream you would get a packet which has no objects/regions/cluts.
>>>> This
>>>>>> packet would turn the subs off, now if this packet was corrupted then
>>>> the
>>>>>> timeout would take care of the removal.
>>>>>
>>>>> I understand that, but this is only so when the source is dvb subs too
>>>>> it can be something else.a different subtitle format for example.
>>>>
>>>> This nb variable doesn't have anything with the numbers of streams.
>>>>
>>>> It ensures that in case of DVB output it would write 2 packets.
>>>> The first one with the subtitles and the second one with no
>>>> regions/segments/cluts.
>>>> It does call avcodec_encode_subtitles() twice, so it does get
>>>> different packets (I hope).
>>>> It is kind of running the whole function twice in a row.
>>>>
>>>> It seems that the dvb encoder have variable "hide_state" that always
>>>> inverts itself when called, so every second encoded packet would be
>>>> "hide" packet. And It seems that removing this variable is part of
>>>> your patch.
>>>>
>>>> So I guess the question is. If the source (input) subtitles are of the
>>>> type that have both valid start and end time, how do you output 2
>>>> packets (one to show and one to hide) ?
>>>> Well, I guess this is mostly problem for the time when we have
>>>> ass/ssa->dvb rendering.
>>>
>>> i think your all missing the point, there is no such thing as an off in
>>> dvb
>>> subtitles. what you have is a set of display/regions/cluts and objects
>>> that
>>> make up a subtitle frame. If there are not object/cluts/regions then in
>>> theory you have an off frame. it does not always work that you have a on
>>> followed by an off. If you look at the bbc_small.ts i uploaded you can
>>> see
>>> that it draws 2 regions, 1 is a full width region with a single object,
>>> the
>>> 2nd line is made up of multiple objects as the new text is added.
>>
>>Yes. This is what we call subtitle hiding - displaying off frame.
>>
>>There are text subtitle formats like sami, that have only start time
>>and no duration. You need to display an empty line if you want to hide
>>the text from the previous line. While DVB subtitles allow more
>>complicated rendering they work on same principle.
>>
>>I think this is also the analogy Michael had in mind.
>>
>>> So in this case you get 6 packets before we get a new region whch will
>>> cause
>>> a full redraw, no where in this stream is an "off" packet, so the
>>> inverting
>>> of the hide_state is NOT CORRECT and is not the correct way of doing dvb
>>> subtitles.
>>
>>I don't think there is anybody who would disagree with you.
>>The old method is hack, it seems to relay on the fact that the whole
>>subtitle could be coded in a single packet. It should be replaced by
>>proper solution. That's however a little tricky. (e.g. the encoder api
>>can't output more than one packet at the moment).
>>
>>> Also you will have noticed that i made changes to how the resetting and
>>> clearing of the cluts/objects etc are performed only as to only reset
>>> when
>>> the main display mode changes.
>>>
>>> ass/ssa -> dvb subtitles
>>>
>>> Well i did post a message 2 weeks ago asking about is, no replys, as to
>>> if
>>
>>Do you refer to the "Hard subtitles" thread? IMHO it is a different issue.
>>
>>> there was some way to do what you mentioned ass/ssa to dvb subtitles. In
>>> this case all that needs to be done is that when you get the off time
>>> from
>>> the input you send to the encoder a 0 region subtitle block, the dvb
>>> subtitles encoder will then output an "off" packet.
>>
>>Yes, I think this is exactly what the problem is circling around.
>>It could be done, but there is no code that does it at the moment.
>>
>>Now, the whole things is a little bit more complicated if you consider
>>that the subtitle code should handle other types of subtitles than
>>DVB.
>>To illustrate the problems more clearly I'll be using the following
>> convention.
>>#1 type. In this mode the packet have only one timestamp, in
>>AVSubtitle this would be start_display_time. In order to show subtitle
>>line you need one AVSubtitle, and to hide it you need another. In
>>short it's the DVB and Sami case.
>>#2 type. In this mode there are two timestamps available. In
>>AVSubtitle they would be start/end_display_time. This is the
>>srt/ass/DVD(vobsub)/xsub etc...
>>
>>I would assume the case where we have input:output as #1#1 and #2#2
>>to be trivial cases. (In your changes you seem to assume ffmpeg should
>>work in #1#1 while at the moment it does work as #2#2).
>>
>>The most troublesome case is #1#2. We can't output encoded packet
>>before we know the end time. This means we need to wait for a second
>>input subtitle packet (or timeout), before we can output the first
>>encoded. This introduces a lag, quite bit lag as subtitles have
>>(relatively) huge durations. So the first output subtitle packet in
>>normal case would appear about 2 seconds after the corresponding audio
>>and video packets have been fed to the muxer. Have in mind that video
>>packets also have their lag, but it is only 1 or 2 frames.
>>
>
> Why do we need to wait for another packet? you have the timeout value, this
In the sami case, It may not have any timeout value.
> added to your start_time gives the end_time. Does this cause a problem is
> another packet comes in with a start_time less than the last packets
> end_time, for example
> in #1 start_time 1.0s timeout 30.0s
> out #2 start_time 1.0s end_time 31.0s
> in #1 start_time 5.0s timeout 30.0s
> out #2 start_time 5.0s end_time 35.0s
First, page_time_out is not meant to be used as duration. It is in
whole seconds and allows +5s delay before triggering.
Second, at least with text subtitles there exists so called
"overlapped subtitles". e.g.
0:0 -> 0:2 "I want to tell you...."
0:1 -> 0:3 "No, no, no."
And for the time these subtitles time overlap (0:1-0:2) both would be visible.
While for DVB design this is not issue (I think it supports native
composition), it could be issue in the general case.
> Now the other way you would subtract the two to get the timeout
> in #2 start_time 1.0s end_time 2.3s
> out #1 start_time 1.0s timeout 2.6s (double just for an extra bit of
> space)in #2 start_time 5.0s end_time 7.3s
> out #2 start_time 5.0s timeout 5.2s
in #2 out #1 is described bellow as #2#1 case.
What I have in mind as description for #1#2 working is:
in #1 start_time=1.0
in #1 start_time=5.0
in #1 start_time=9.0
out #2 start_time=1.0 end_time=5.0
out #2 start_time=5.0 end_time=9.0
...
>>The less troublesome case is #2#1. We have both times and the subtitle
>
>>encoder should output 2 packets. One for the start time with the text
>>and one with the empty text/image at the end time.
>>
>>--
>>Now, about your patch. If I understand it correctly assumes the #1#1
>>for proper implementation.
>>
>>However if you insist that the decoders should always output #1 type
>>subtitles, the #2 type encoder will be forced into the #1#2 case, even
>>when the source subtitles are #2 type. I'd like to avoid that.
>
> Nope i dont assume this, this is how it works. You send a subtitle frame to
> the encoder and you will get what you send, a packet with subtitles or an
> empty packet.
>
>
>>
>>On the other hand, if we insist on all decoders always output #2 type
>>subtitles, we are faced by the same lag as in #1#2 case, but this time
>>in the decoder. It would happen even when we have #1#1 chain. Even
>>worse, such lag would be quite troublesome for video players that use
>>only the decoder. I'd say avoid at all cost.
>>
>>So there are 2 proper solutions. simple and complicated.
>>The complicated one is implementing support for handling the #1#2 case
>>lag. I don't think I can ask that from you.
>
> i dont see that problem occuring, the time lag wont appear, the #1 packet
> comes in and we use the timeout, see above
>
>>
>>The simple solution is to make at least the other 3 modes operational.
>>Here are some steps that are needed.
>>
>>1. Find a way to signal #1 or #2 type.
>>Simple solution would be setting end_display_time=0. Right now the dvb
>>decoder fills it with timeout value.
>>Another way is to add new flag field to AVSubtitle. I recommend
>>uint16_t value right after the "uint16_t format" as it may not even
>>break ABI due to struct members alignment.
>>
>>2. Make the dvb decoder output #1 type subtitles.
>>
>>3. Make the dvb encoder output one additional packet with #2 type subtitle.
>>
>>3.1. The dvb encoder should check if the AVSubtitle is #2 type, and if
>>it is so, at the end output a packet with pts of end_display_time and
>>empty subtitle. In essence instead of having "hide_state" iterate at
>>every call, the variable should keep the #2 type flag and output empty
>>packet if it is fed with empty subtitle.
>>
>>3.2. The function do_subtitle_out() in ffmpeg.c should be fixed so it
>>would call subtitle encoder once with the AVSubtitle packet and then
>>loop at encoder (with empty sub) & writing packets until encoder
>>stops producing data. A number of problems here. the
>>avcodec_encode_subtitle() doesn't return packets, so the encoder can't
>>adjust pts on its own. No "got_picture" variable either.
>>So you may pick the easy road (just make it work) or go the highway
>>(create avcodec_encode_subtitle2() that return packets. so the loops
>>turns into
>> for(;;){
>> if (avcodec_subtitle_encode2(enc, &pkt, sub) <= 0 ) break;
>> write_frame(s,&pkt, ...);
>> }
>>
>>---
>>
>>The proposed changes should probably be discussed with the other
>>developers before agreeing on final implementation.
>>
>>
>>Sorry for the long delay. I've never ever messed with the subtitles
>>code before and I don't even know how to use it. So I needed some time
>>to look up the internal working. And then I needed some more time to
>>figure out the possible correct solutions.
>>
>>I would understand if you loose patience, but we are just trying to
>>don't break one thing while fixing another.
>>
>>Your work is really appreciated and it is great that there is somebody
>>interested in fixing code that haven't been touched for over 6 years.
>>Just don't give up (yet:)
>>
>>Best Regards.
>> Ivan Kalvachev
> Ok ill wait on your talks but in the meantime im moving on to finishing my
> modifications, if they dont make it in then hey.
>
> But i need to start looking into adding "hard subs" so once im happy i have
> everything done im moving onto the that.
>
> Once and you have decided on your direction of changes let me know and i
> will try and help with the dvbsubs engine, either in code or knowledge of
> how the system works.
>
> Its a pity that removing around 12 lines of code will at least allow dvbsubs
> to be encoded and not cause a crash.
>
> Is there/has there been any work on getting something that would allow
> transcoding of subtitles from differing formats yet, as this would be a good
> way for me to try and see the problems you mention.
By looking at the code it seems that dvdsub* and xsub* are both #2
type. I would advice you to test dvd->dvb and dvb->dvd conversion, as
we would expect it to be quite common.
More information about the ffmpeg-devel
mailing list