[FFmpeg-devel] [PATCH] DVB Subtitles Fix
Michael Niedermayer
michaelni at gmx.at
Wed May 4 16:14:20 CEST 2011
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 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?
[...]
> @@ -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?
[...]
> @@ -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
> @@ -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
[...]
> @@ -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?
> 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110504/09dec33e/attachment.asc>
More information about the ffmpeg-devel
mailing list