[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