[FFmpeg-devel] IEC61937 compatible muxer

[Bartlomiej Wolowiec]

Friday 07 August 2009 21:59:05 Reimar D?ffinger napisa?(a):
> 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.

Ok, I use AV_RB32.

[...]

> > +    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)

Hmm... blocks << 5 is a real code instruction, when 2048 >> 5 is a constant ?

> > +    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).
>

I changed it to put_be16. Padding value depend on used codec... I don't think 
its a speed-relevant.

-- 
Bartlomiej Wolowiec
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch
Type: text/x-diff
Size: 9695 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090808/399207ed/attachment.diff>