[FFmpeg-devel] [PATCH] DVB Subtitles Fix
JULIAN GARDNER
joolzg at btinternet.com
Fri May 6 14:36:28 CEST 2011
>________________________________
>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 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
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
>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.
joolz
More information about the ffmpeg-devel
mailing list