[FFmpeg-devel] [PATCH]Basic XSUB encoder (take 3)

[Björn Axelsson]

On Thu, 5 Feb 2009, Reimar D?ffinger wrote:

> On Thu, Feb 05, 2009 at 12:07:56AM +0100, Bj?rn Axelsson wrote:
> > +/** Encode a single color run. At most 16 bits will be used. */
> > +void put_xsub_rle(PutBitContext *pb, int len, int color)
>
> static

Of course...

> > +{
> > +    if (len <= 255)
> > +        put_bits(pb, 2 + ((ff_log2_tab[len] >> 1) << 2), len);
> > +    else
> > +        put_bits(pb, 14, 0);
> > +    put_bits(pb, 2, color);
>
> If you add a
> > if (!len) return;
> you would not need most of the PADDING checks.

Well, there are only two checks and one is constant and can be optimized
away by the compiler.

> Not sure if it is better, but this could also be written as
> > int bits = 14;
> > if (len > 255)
> >     len = 0;
> > else
> >     bits = 2 + ((ff_log2_tab[len] >> 1) << 2);
> > put_bits(pb, bits, len);

Personally I think the current code is clearer. And I can't see any
obvious speed difference.

> > +            // One run + padding will need 3 bytes in worst case
> > +            if (bufsize - ((put_bits_count(&pb) + 7) >> 3) < 3)
>
> The worst case is 3 runs + padding.

I disagree.
Worst case bit usage is 4 bits left padding (with PADDING <= 2), 16
bits main run, 4 bits right padding and 4 bits for byte alignment.
If left padding is needed, a new row begins byte-aligned and the 4 bits
for alignment are not used, which gives 24 bits.
If left padding is not needed, at most 20 bits will be encoded and with
alignment that gives us also 24 bits at most.

> > +    // Buffer large enough to hold static header?
> > +    if (bufsize < 27 + 7*2 + 4*3) {
> > +        av_log(avctx, AV_LOG_ERROR, "Buffer too small for XSUB header.\n");
> > +        return -1;
> > +    }
> [...]
> > +    // Bitmap
> > +    q = hdr;
> > +    if (xsub_encode_rle(&q, bufsize - (q-buf),
> > +                h->rects[0]->pict.data[0],
> > +                h->rects[0]->pict.linesize[0]*2,
> > +                h->rects[0]->w, (h->rects[0]->h + 1) / 2))
> > +        return -1;
> > +    bytestream_put_le16(&rlelenptr, q-hdr); // Length of first field
> > +
> > +    if (xsub_encode_rle(&q, bufsize - (q-buf),
> > +            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))
> > +        return -1;
> > +
> > +    // Enforce total height to be be multiple of 2
> > +    if (h->rects[0]->h & 1) {
> > +        if (bufsize - (q - buf) < 2)
> > +            return -1;
> > +        bytestream_put_le16(&q, PADDING_COLOR); // RLE empty line
> > +    }
>
> you can avoid this check by doing at the very beginning
> > bufsize -= 27 + 7*2 + 4*3 + 2;
> > if (bufsize <= 0) {
> ...
> and using
> > xsub_encode_rle(&q, bufsize - (q-hdr),

Hmm. It can also be done without wasting the 2 bytes if not needed:
bufsize -= 27 + 7*2 + 4*3 + 2*(h->rects[0]->h & 1);

> It would be possible to save one byte for small subtitles by using
> put_xsub_rle to write the final line, but that might get ugly -
> at least unless you pass the put_bits context to xsub_encode_rle.

I actually started doing that, but it got very ugly. I don't think it is
worth it for a single byte in a very uncommon case.

-- 
Bj?rn Axelsson