[FFmpeg-devel] IEC61937 compatible muxer

Reimar Döffinger Reimar.Doeffinger
Fri Aug 7 21:59:05 CEST 2009


Hello,
mostly a bit of cosmetic nit-picking...

On Fri, Aug 07, 2009 at 08:25:07PM +0200, Bartlomiej Wolowiec wrote:
> +    uint32_t syncword_dts = be2me_32(*(uint32_t *) pkt->data);

Hm. If performance is not critical, AV_RB32 has better readability IMO.
Actually a temporary variable + bytestream_get_be32 might be even
better.

> +    switch (syncword_dts) {
> +    case DCA_MARKER_RAW_BE:
> +        blocks =
> +            (((pkt->data[4] & 0x01) << 6) | (pkt->data[5] >> 2)) + 1;
> +        break;
> +    case DCA_MARKER_RAW_LE:
> +        blocks =
> +            (((pkt->data[5] & 0x01) << 6) | (pkt->data[4] >> 2)) + 1;
> +        break;
> +    case DCA_MARKER_14B_BE:
> +        blocks =
> +            (((pkt->data[5] & 0x07) << 4) | ((pkt->data[6] & 0x3f) >> 2)) + 1;
> +        break;
> +    case DCA_MARKER_14B_LE:
> +        blocks =
> +            (((pkt->data[4] & 0x07) << 4) | ((pkt->data[7] & 0x3f) >> 2)) + 1;
> +        break;
> +    default:
> +        av_log(s, AV_LOG_ERROR, "bad DTS syncword\n");
> +        return -1;
> +    }

Hm, the + 1 could be moved outside the switch, and the first two are the
same as
((AV_RB16(pkt->data + 4) >> 2) & 0x7f) + 1;
((AV_RL16(pkt->data + 4) >> 2) & 0x7f) + 1;

> +    av_log(s, AV_LOG_DEBUG, "blocks=%i\n", blocks);
> +    switch (blocks) {
> +    case 512 >> 5:
> +        ctx->data_type = IEC958_DTS1;
> +        break;
> +    case 1024 >> 5:
> +        ctx->data_type = IEC958_DTS2;
> +        break;
> +    case 2048 >> 5:
> +        ctx->data_type = IEC958_DTS3;
> +        break;
> +    default:

Why not switch(blocks << 5) (can't "overflow" AFAICT)

> +    ctx->pkt_size = ((pkt->size + 1) >> 1) << 4;

FFALIGN(pkt->size, 2) * 8;
might be clearer?

> +    ret = (*ctx->header_info) (s, pkt);

*shudder*. Why not just
> +    ret = ctx->header_info(s, pkt);

> +    put_buffer(s->pb, pkt->data, pkt->size & (~1));

Pointless () around ~1

> +    if (pkt->size & 1)
> +        put_be16(s->pb, pkt->data[pkt->size - 1]);
> +
> +    for (; padding > 0; padding--)
> +        put_le16(s->pb, 0);

Using put_le16 here is inconsistent even if it is slightly faster
on little-endian machines. IMO use put_be16 for consistency,
if it is speed-relevant, either a new avio function could be added
or put_byte or memset+put_buffer could be used (I have no idea what the
usual/minimum/maximum values for padding are, which method is most
effective would depend on that).

> +    NULL,
> +    //.flags=

Both seem pointless.



More information about the ffmpeg-devel mailing list