[FFmpeg-devel] IEC61937 compatible muxer

Michael Niedermayer michaelni
Mon Aug 10 13:38:56 CEST 2009


On Sat, Aug 08, 2009 at 12:22:45AM +0200, Bartlomiej Wolowiec wrote:
> 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.

[...]

> +#define IEC958_AC3                0x01
> +#define IEC958_MPEG1_LAYER1       0x04
> +#define IEC958_MPEG1_LAYER23      0x05

> +//#define IEC958_MPEG2_EXT          0x06 /* With extension */

why is this commented out?
and its not doxygen comp
and it should maybe be an enum


> +#define IEC958_MPEG2_AAC          0x07
> +#define IEC958_MPEG2_LAYER1_LSF   0x08  /* Low Sampling Frequency */
> +#define IEC958_MPEG2_LAYER2_LSF   0x09  /* Low Sampling Frequency */
> +#define IEC958_MPEG2_LAYER3_LSF   0x0A  /* Low Sampling Frequency */
> +#define IEC958_DTS1               0x0B
> +#define IEC958_DTS2               0x0C
> +#define IEC958_DTS3               0x0D
> +#define IEC958_MPEG2_AAC_LSF_2048 0x13
> +#define IEC958_MPEG2_AAC_LSF_4096 (0x13|0x20)
> +//#define IEC958_EAC3               0x15
> +
> +typedef struct IEC958Context {

> +    int data_type;              ///< Burst info

too terse comment


> +    int pkt_size;               ///< Length code (number of bits or bytes - according to data_type)
> +    int pkt_offset;             ///< Repetition period of a data burst in bytes

> +    int (*header_info) (AVFormatContext *s, AVPacket *pkt);

should be documented


[...]
> +    {
> +        //XXX swab... ?
> +        uint16_t *data = (uint16_t *) pkt->data;
> +        int i;
> +        for (i = 0; i < pkt->size >> 1; i++)
> +            put_be16(s->pb, data[i]);
> +    }

should probably be swaped in memory and then written at once


[...]
> Zmiany atrybut?w dla: libavformat/spdif.c
> ___________________________________________________________________
> Dodane: svn:special
>    + *

hmm


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20090810/e8ba483f/attachment.pgp>



More information about the ffmpeg-devel mailing list