[FFmpeg-devel] [PATCH] DVB Subtitles Fix
JULIAN GARDNER
joolzg at btinternet.com
Wed May 4 17:11:33 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, 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.
>
>>
>> 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.
>
> [...]
>
>> @@ -205,9 +206,6 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
>>
>> page_id = 1;
>>
>> - if (h->num_rects == 0 || h->rects == NULL)
>> - return -1;
>> -
>> *q++ = 0x00; /* subtitle_stream_id */
>>
>> /* page composition segment */
>
> why is this removed?
>
No regions is NOT AN ERROR, and returning -1 causes ffmpeg to drop out with a error
>
> [...]
>> @@ -342,7 +338,7 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
>> bytestream_put_be16(&q, object_id);
>> *q++ = (s->object_version << 4) | (0 << 2) | (0
> << 1) | 1; /* version = 0,
>>
> onject_coding_method,
>> -
> non_modifying_color_flag */
>> +
> non_modifying_color_flag */
>> {
>> uint8_t *ptop_field_len, *pbottom_field_len, *top_ptr,
> *bottom_ptr;
>> void (*dvb_encode_rle)(uint8_t **pq,
>> @@ -364,7 +360,7 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
>> bottom_ptr = q;
>> dvb_encode_rle(&q,
> h->rects[object_id]->pict.data[0] + h->rects[object_id]->w,
>> h->rects[object_id]->w * 2,
> h->rects[object_id]->w,
>> - h->rects[object_id]->h >>
> 1);
>> + h->rects[object_id]->h
>>> 1);
>>
>> bytestream_put_be16(&ptop_field_len, bottom_ptr -
> top_ptr);
>> bytestream_put_be16(&pbottom_field_len, q -
> bottom_ptr);
>
> these 2 hunks should not be in the patch
>
ok my mistake
>
>> @@ -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.
>
>
> [...]
>> @@ -1001,15 +1040,14 @@ static void
> dvbsub_parse_region_segment(AVCodecContext *avctx,
>>
>> const uint8_t *buf_end = buf + buf_size;
>> int region_id, object_id;
>> + int version;
>> DVBSubRegion *region;
>> DVBSubObject *object;
>> DVBSubObjectDisplay *display;
>> int fill;
>>
>> - if (buf_size < 10)
>> - return;
>> -
>
> why is this removed?
Again buf_size has already been validated at the loop processing the data
>
>
>> region_id = *buf++;
>> + buf_size--;
>>
>> region = get_region(ctx, region_id);
>>
>> @@ -1017,17 +1055,22 @@ static void
> dvbsub_parse_region_segment(AVCodecContext *avctx,
>> region = av_mallocz(sizeof(DVBSubRegion));
>>
>> region->id = region_id;
>> + region->version = -1;
>>
>> region->next = ctx->region_list;
>> ctx->region_list = region;
>> }
>>
>> + version = ((*buf)>>4) & 15;
>> fill = ((*buf++) >> 3) & 1;
>> + buf_size--;
>>
>> region->width = AV_RB16(buf);
>> buf += 2;
>> + buf_size -= 2;
>> region->height = AV_RB16(buf);
>> buf += 2;
>> + buf_size -= 2;
>>
>> if (region->width * region->height != region->buf_size) {
>> av_free(region->pbuf);
>> @@ -1035,26 +1078,33 @@ static void
> dvbsub_parse_region_segment(AVCodecContext *avctx,
>> region->buf_size = region->width * region->height;
>>
>> region->pbuf = av_malloc(region->buf_size);
>> -
>> - fill = 1;
>> + region->dirty = 0;
>> }
>>
>> region->depth = 1 << (((*buf++) >> 2) & 7);
>> + buf_size--;
>> if(region->depth<2 || region->depth>8){
>> av_log(avctx, AV_LOG_ERROR, "region depth %d is
> invalid\n", region->depth);
>> region->depth= 4;
>> }
>> region->clut = *buf++;
>> + buf_size--;
>>
>> - if (region->depth == 8)
>> + if (region->depth == 8) {
>> region->bgcolor = *buf++;
>> + buf_size--;
>> + buf += 1;
>> + buf_size--;
>> + }
>> else {
>> buf += 1;
>> + buf_size--;
>>
>> if (region->depth == 4)
>> region->bgcolor = (((*buf++) >> 4) & 15);
>> else
>> region->bgcolor = (((*buf++) >> 2) & 3);
>> + buf_size--;
>> }
>>
>> av_dlog(avctx, "Region %d, (%dx%d)\n", region_id,
> region->width, region->height);
>> @@ -1066,9 +1116,11 @@ static void
> dvbsub_parse_region_segment(AVCodecContext *avctx,
>>
>> delete_region_display_list(ctx, region);
>>
>> - while (buf + 5 < buf_end) {
>> +// while (buf + 5 < buf_end) {
>> + while (buf_size) {
>
> why all these changes?
> the loop surely will not work with a single byte and
> maintaining a seperate buf_size variable in addition to the buf &
> buf_end pointers seems unneeded
>
Again buf_size has already been validated at the loop processing the data
More information about the ffmpeg-devel
mailing list