[FFmpeg-devel] [PATCH] DVB Subtitles Fix
JULIAN GARDNER
joolzg at btinternet.com
Wed May 4 20:18:12 CEST 2011
----- 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.
>>
>> >
>> >>
>> >> 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.
>
>
> [...]
>
>> >
>> >> @@ -387,7 +383,7 @@ static int
> encode_dvb_subtitles(DVBSubtitleContext *s,
>> >> *q++ = 0xff; /* end of PES data */
>> >>
>> >> s->object_version = (s->object_version + 1) & 0xf;
>> >> - s->hide_state = !s->hide_state;
>> >> +
>> >> return q - outbuf;
>> >> }
>> >>
>> >> @@ -399,6 +395,11 @@ static int dvbsub_encode(AVCodecContext
> *avctx,
>> >> int ret;
>> >>
>> >> ret = encode_dvb_subtitles(s, buf, sub);
>> >> +
>> >> +#ifdef DEBUG
>> >> + av_log( avctx, AV_LOG_INFO, "dvbsub_encode %d
>> > bytes\r\n", ret);
>> >> +#endif
>> >> +
>> >> return ret;
>> >> }
>> >>
>> >> diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
>> >> index 288e6f5..d9c396a 100644
>> >> --- a/libavcodec/dvbsubdec.c
>> >> +++ b/libavcodec/dvbsubdec.c
>> >> @@ -157,6 +157,7 @@ static void png_save2(const char *filename,
> uint32_t
>> > *bitmap, int w, int h)
>> >>
>> >> typedef struct DVBSubCLUT {
>> >> int id;
>> >> + int version;
>> >>
>> >> uint32_t clut4[4];
>> >> uint32_t clut16[16];
>> >> @@ -183,6 +184,7 @@ typedef struct DVBSubObjectDisplay {
>> >>
>> >> typedef struct DVBSubObject {
>> >> int id;
>> >> + int version;
>> >>
>> >> int type;
>> >>
>> >> @@ -202,6 +204,7 @@ typedef struct DVBSubRegionDisplay {
>> >>
>> >> typedef struct DVBSubRegion {
>> >> int id;
>> >> + int version;
>> >>
>> >> int width;
>> >> int height;
>> >> @@ -212,6 +215,7 @@ typedef struct DVBSubRegion {
>> >>
>> >> uint8_t *pbuf;
>> >> int buf_size;
>> >> + int dirty;
>> >>
>> >> DVBSubObjectDisplay *display_list;
>> >>
>> >> @@ -231,6 +235,7 @@ typedef struct DVBSubContext {
>> >> int composition_id;
>> >> int ancillary_id;
>> >>
>> >> + int version;
>> >> int time_out;
>> >> DVBSubRegion *region_list;
>> >> DVBSubCLUT *clut_list;
>> >
>> >
>> >> @@ -321,21 +326,10 @@ static void
> delete_region_display_list(DVBSubContext
>> > *ctx, DVBSubRegion *region)
>> >>
>> >> }
>> >>
>> >> -static void delete_state(DVBSubContext *ctx)
>> >> +static void delete_cluts(DVBSubContext *ctx)
>> >> {
>> >> - DVBSubRegion *region;
>> >> DVBSubCLUT *clut;
>> >>
>> >> - while (ctx->region_list) {
>> >> - region = ctx->region_list;
>> >> -
>> >> - ctx->region_list = region->next;
>> >> -
>> >> - delete_region_display_list(ctx, region);
>> >> - av_free(region->pbuf);
>> >> - av_free(region);
>> >> - }
>> >> -
>> >> while (ctx->clut_list) {
>> >> clut = ctx->clut_list;
>> >>
>> >> @@ -343,12 +337,35 @@ static void delete_state(DVBSubContext
> *ctx)
>> >>
>> >> av_free(clut);
>> >> }
>> >> +}
>> >>
>> >> - av_freep(&ctx->display_definition);
>> >> +static void delete_objects(DVBSubContext *ctx)
>> >> +{
>> >> + DVBSubObject *object;
>> >> +
>> >> + while (ctx->object_list) {
>> >> + object = ctx->object_list;
>> >> +
>> >> + ctx->object_list = object->next;
>> >> +
>> >> + av_free(object);
>> >> + }
>> >> +}
>> >>
>> >> - /* Should already be null */
>> >> - if (ctx->object_list)
>> >> - av_log(0, AV_LOG_ERROR, "Memory deallocation
>> > error!\n");
>> >> +static void delete_regions(DVBSubContext *ctx)
>> >> +{
>> >> + DVBSubRegion *region;
>> >> +
>> >> + while (ctx->region_list) {
>> >> + region = ctx->region_list;
>> >> +
>> >> + ctx->region_list = region->next;
>> >> +
>> >> + delete_region_display_list(ctx, region);
>> >> +
>> >> + av_free(region->pbuf);
>> >> + av_free(region);
>> >> + }
>> >> }
>> >>
>> >> static av_cold int dvbsub_init_decoder(AVCodecContext *avctx)
>> >> @@ -365,6 +382,8 @@ static av_cold int
> dvbsub_init_decoder(AVCodecContext
>> > *avctx)
>> >> ctx->ancillary_id = AV_RB16(avctx->extradata +
> 2);
>> >> }
>> >>
>> >> + ctx->version = -1;
>> >> +
>> >> default_clut.id = -1;
>> >> default_clut.next = NULL;
>> >>
>> >> @@ -433,7 +452,13 @@ static av_cold int
> dvbsub_close_decoder(AVCodecContext
>> > *avctx)
>> >> DVBSubContext *ctx = avctx->priv_data;
>> >> DVBSubRegionDisplay *display;
>> >>
>> >> - delete_state(ctx);
>> >> + delete_regions(ctx);
>> >> +
>> >> + delete_objects(ctx);
>> >> +
>> >> + delete_cluts(ctx);
>> >> +
>> >> + av_freep(&ctx->display_definition);
>> >>
>> >> while (ctx->display_list) {
>> >> display = ctx->display_list;
>> >
>> > why is this changeset done?
>> > this looks like just reformating code and then adding delete_objects()
>> > later can be done without the reformating
>>
>> Because we need to be able to delete certain things depending on its
> version, i still have some changes to make as in, what happens when an objects
> version number changes, only the object needs deleting the same as with cluts.
>
> ok
>
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Breaking DRM is a little like attempting to break through a door even
> though the window is wide open and the only thing in the house is a bunch
> of things you dont want and which you would get tomorrow for free anyway
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
any more questions just ask, i do know a little about dvb subs :-)
joolz
More information about the ffmpeg-devel
mailing list