[FFmpeg-devel] [PATCH]Basic XSUB encoder (take 5)
Michael Niedermayer
michaelni
Fri Feb 6 02:59:15 CET 2009
On Thu, Feb 05, 2009 at 10:45:25PM +0100, Bj?rn Axelsson wrote:
> On Thu, 5 Feb 2009, Reimar D?ffinger wrote:
>
> > On Thu, Feb 05, 2009 at 10:04:18PM +0100, Bj?rn Axelsson wrote:
> > > + // Enforce total height to be be multiple of 2
> > > + if (h->rects[0]->h & 1)
> > > + put_xsub_rle(&pb, h->rects[0]->w, PADDING_COLOR);
> >
> > Is there a buffer size check for that line of code?
>
> Yes, it is hidden in the call to init_put_bits().
> Comment added for some additional clarity.
>
> > > + flush_put_bits(&pb);
> > > +
> > > + return hdr - buf + put_bits_count(&pb)/8;
> >
> > You need another align_put_bits or you might return one too few here.
>
> Oops.
>
> > I think I have no further comment, I'd appreciate if others could review
> > the build system and other stuff...
>
> Thank you for your time.
>
> I still need some help solving the timestamp problem as I don't think the
> hack used in the patch is acceptable.
i thought about this a little, but i think your hack is not that far from
a possible solution
look at AVFrame.pts & AVCodecContext.time_base
AVSubtitle.pts should be like AVFrame.pts
also start_display_time must be 0 on encoding because not all subtitle
encoders support it. Actually i think start_display_time should be handled
somehow different but iam not sure also this is getting off topic i guess
a patch adding a check in avcodec_encode_subtitle() that start_display_time is
0 is welcome!
[...]
> Index: libavcodec/xsubenc.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ libavcodec/xsubenc.c 2009-02-05 22:40:30.000000000 +0100
> @@ -0,0 +1,214 @@
> +/*
> + * DivX (XSUB) subtitle encoder
> + * Copyright (c) 2005 DivX, Inc.
> + * Copyright (c) 2009 Bjorn Axelsson
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "avcodec.h"
> +#include "bytestream.h"
> +#include "bitstream.h"
> +
> +/* For obscure reasons we pad the subtitles with two pixels on either side */
> +#define PADDING 2
> +#define PADDING_COLOR 0
set this to 0 please
why?
id like to know what these obscure reasons are, and thats the quickest way
to find out
> +
> +/** Encode a single color run. At most 16 bits will be used. */
> +static void put_xsub_rle(PutBitContext *pb, int len, int color)
> +{
> + if (len <= 255)
> + put_bits(pb, 2 + ((ff_log2_tab[len] >> 1) << 2), len);
do you really have to access the table directly? cant av_log2() be used?
> + else
> + put_bits(pb, 14, 0);
> + put_bits(pb, 2, color);
> +}
> +
> +/** Encode a 4-colour bitmap with XSUB rle.
> + * The encoded bitmap may be wider than the source bitmap due to padding. */
/**
* first line
* second line
*/
looks nicer IMHO
> +static int xsub_encode_rle(PutBitContext *pb,
> + const uint8_t *bitmap, int linesize, int w, int h)
that line is ugly broken
> +{
> + int x0, x1, y, len, color = PADDING_COLOR;
> +
> + for (y = 0; y < h; y++) {
> + x0 = 0;
> + while (x0 < w) {
> + // Make sure we have enough room for at least one run and padding
> + if (pb->size_in_bits - put_bits_count(pb) < 7*8)
> + return -1;
> +
> + x1 = x0;
> + color = bitmap[x1++] & 3;
> + while (x1 < w && (bitmap[x1] & 3) == color)
> + x1++;
> + len = x1 - x0;
> + if (PADDING && x0 == 0) {
> + if (color == PADDING_COLOR) {
> + len += PADDING;
> + x0 -= PADDING;
> + } else
> + put_xsub_rle(pb, PADDING, PADDING_COLOR);
> + }
> +
> + // Run can't be longer than 255, unless it is the rest of a row
> + if (x1 == w && color == PADDING_COLOR)
> + len += PADDING + (w&1);
> + else
a {} between if/else costs no lines but can make future patches shorter
> + len = FFMIN(len, 255);
> + put_xsub_rle(pb, len, color);
> +
> + x0 += len;
> + }
> + if (color != PADDING_COLOR && (PADDING || (w&1)))
> + put_xsub_rle(pb, PADDING + (w&1), PADDING_COLOR);
> +
> + align_put_bits(pb);
> +
> + bitmap += linesize;
> + }
> +
> + return 0;
> +}
> +
> +static const int tc_divs[3] = { 1000, 60, 60 };
only used in the next function so can be inside the function
> +static int make_tc(uint64_t ms, int *tc)
> +{
> + int i;
> + for (i=0; i<3; i++) {
> + tc[i] = ms % tc_divs[i];
> + ms /= tc_divs[i];
> + }
> + tc[3] = ms;
> + return ms > 99;
> +}
> +
> +static int xsub_encode(AVCodecContext *avctx, unsigned char *buf,
> + int bufsize, void *data)
> +{
> + AVSubtitle *h = (AVSubtitle *)data;
^^^^^^^^^^^^^^
useless
> + uint64_t startTime = h->pts / 1000; // FIXME: need better solution...
> + uint64_t endTime = startTime + h->end_display_time - h->start_display_time;
> + int start_tc[4], end_tc[4];
> + uint8_t *hdr = (uint8_t *)buf + 27; // Point behind the timestamp
^^^^^^^^^^^
useless
> + uint8_t *rlelenptr;
> + uint16_t width, height;
> + int i;
> + PutBitContext pb;
> +
> + if (h->num_rects == 0 || h->rects == NULL)
!h->rects
> + return -1;
also this check can be moved to avcodec_encode_subtitle() i think
> +
> + if (bufsize < 27 + 7*2 + 4*3) {
> + av_log(avctx, AV_LOG_ERROR, "Buffer too small for XSUB header.\n");
> + return -1;
> + }
> +
> + // TODO: support multiple rects
> + if (h->num_rects > 1)
> + av_log(avctx, AV_LOG_WARNING, "Only single rects supported (%d in subtitle.)\n", h->num_rects);
> +
> + // TODO: render text-based subtitles into bitmaps
> + if (!h->rects[0]->pict.data[0] || !h->rects[0]->pict.data[1]) {
> + av_log(avctx, AV_LOG_WARNING, "No subtitle bitmap available.\n");
> + return -1;
> + }
> +
> + // TODO: color reduction, similar to dvdsub encoder
> + if (h->rects[0]->nb_colors > 4)
> + av_log(avctx, AV_LOG_WARNING, "No more than 4 subtitle colors supported (%d found.)\n", h->rects[0]->nb_colors);
> +
> + // TODO: Palette swapping if color zero is not transparent
> + if (((uint32_t *)h->rects[0]->pict.data[1])[0] & 0xff)
> + av_log(avctx, AV_LOG_WARNING, "Color index 0 is not transparent. Transparency will be messed up.\n");
> +
> + if (make_tc(startTime, start_tc) || make_tc(endTime, end_tc)) {
> + av_log(avctx, AV_LOG_WARNING, "Time code >= 100 hours.\n");
> + return -1;
> + }
do you have a official player? i so what happens with a larger timecode ?
> +
> + snprintf(buf, 28,
> + "[%02d:%02d:%02d.%03d-%02d:%02d:%02d.%03d]",
> + start_tc[3], start_tc[2], start_tc[1], start_tc[0],
> + end_tc[3], end_tc[3], end_tc[1], end_tc[0]);
> +
> + // Width and height must probably be multiples of 2.
> + // 2 pixels required on either side of subtitle.
> + // Possibly due to limitations of hardware renderers.
> + // TODO: check if the bitmap is already padded
> + width = ((h->rects[0]->w + 1) & ~1) + PADDING * 2;
> + height = (h->rects[0]->h + 1) & ~1;
> +
> + bytestream_put_le16(&hdr, width);
> + bytestream_put_le16(&hdr, height);
> + bytestream_put_le16(&hdr, h->rects[0]->x);
> + bytestream_put_le16(&hdr, h->rects[0]->y);
> + bytestream_put_le16(&hdr, h->rects[0]->x + width);
> + bytestream_put_le16(&hdr, h->rects[0]->y + height);
> +
> + rlelenptr = hdr; // Will store length of first field here later.
> + hdr+=2;
> +
> + // Palette
> + for (i=0; i<4; i++)
> + bytestream_put_be24(&hdr, ((uint32_t *)h->rects[0]->pict.data[1])[i]);
> +
> + // Bitmap
> + // RLE buffer. Reserve 2 bytes for possible padding after the last row.
> + init_put_bits(&pb, hdr, bufsize - (hdr - buf) - 2);
> + if (xsub_encode_rle(&pb,
> + h->rects[0]->pict.data[0],
> + h->rects[0]->pict.linesize[0]*2,
> + h->rects[0]->w, (h->rects[0]->h + 1) / 2))
>>1
> + return -1;
> + bytestream_put_le16(&rlelenptr, put_bits_count(&pb)/8); // Len of first field
>>8
> +
> + if (xsub_encode_rle(&pb,
> + h->rects[0]->pict.data[0] + h->rects[0]->pict.linesize[0],
> + h->rects[0]->pict.linesize[0]*2,
> + h->rects[0]->w, h->rects[0]->h / 2))
>>1
[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h.orig 2009-02-05 21:17:11.000000000 +0100
> +++ libavcodec/avcodec.h 2009-02-05 21:18:31.000000000 +0100
> @@ -30,7 +30,7 @@
> #include "libavutil/avutil.h"
>
> #define LIBAVCODEC_VERSION_MAJOR 52
> -#define LIBAVCODEC_VERSION_MINOR 12
> +#define LIBAVCODEC_VERSION_MINOR 13
> #define LIBAVCODEC_VERSION_MICRO 0
>
> #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> @@ -2432,6 +2432,7 @@
> uint32_t end_display_time; /* relative to packet pts, in ms */
> unsigned num_rects;
> AVSubtitleRect **rects;
> + uint64_t pts; ///< Same as packet pts, in AV_TIME_BASE
int64_t
[...]
> Index: libavformat/avienc.c
> ===================================================================
> --- libavformat/avienc.c.orig 2009-02-05 21:17:03.000000000 +0100
> +++ libavformat/avienc.c 2009-02-05 21:17:36.000000000 +0100
> @@ -82,6 +82,9 @@
> if (type == CODEC_TYPE_VIDEO) {
> tag[2] = 'd';
> tag[3] = 'c';
> + } else if (type == CODEC_TYPE_SUBTITLE) {
> + tag[2] = 's';
> + tag[3] = 'b';
> } else {
> tag[2] = 'w';
> tag[3] = 'b';
> @@ -213,8 +216,10 @@
> case CODEC_TYPE_AUDIO: put_tag(pb, "auds"); break;
> // case CODEC_TYPE_TEXT : put_tag(pb, "txts"); break;
> case CODEC_TYPE_DATA : put_tag(pb, "dats"); break;
> + case CODEC_TYPE_SUBTITLE: put_tag(pb, "vids"); break;
> }
> - if(stream->codec_type == CODEC_TYPE_VIDEO)
> + if(stream->codec_type == CODEC_TYPE_VIDEO
> + || stream->codec_type == CODEC_TYPE_SUBTITLE)
> put_le32(pb, stream->codec_tag);
> else
> put_le32(pb, 1);
this may be wrong as we have a ('t', 'x', 't', 's') in avidec as well so
not all might be vids
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090206/3efe55e9/attachment.pgp>
More information about the ffmpeg-devel
mailing list