[FFmpeg-devel] IEC61937 compatible muxer

[Bartlomiej Wolowiec]

Monday 10 August 2009 13:38:56 Michael Niedermayer napisa?(a):
> 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
>
>
> [...]

I attach improved version of patch.
-- 
Bartlomiej Wolowiec
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch
Type: text/x-diff
Size: 12404 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090815/16eca18e/attachment.diff>