[FFmpeg-devel] IEC61937 compatible muxer

Michael Niedermayer michaelni
Mon Aug 17 02:43:53 CEST 2009


On Sat, Aug 15, 2009 at 10:31:08PM +0200, Bartlomiej Wolowiec wrote:
> 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.
[...]
> +
> +typedef struct IEC958Context {
> +    enum IEC958DataType data_type;  ///< Burst info - reference to type of payload of the data-burst

> +    int pkt_size;                   ///< Length code (number of bits or bytes - according to data_type)

it does not appear that it can be both currently, am i missing
something?


> +    int pkt_offset;                 ///< Repetition period of data burst in bytes

is it just me or is the doxy unclear?


> +    uint8_t *buffer;                ///< Allocated buffer, used for swap bytes
> +    int buffer_size;                ///< Size of allocated buffer


> +    /// Function, which generates codec dependent header information
> +    int (*header_info) (AVFormatContext *s, AVPacket *pkt);

this too is a little unclear
also i would add a empty line before /// ...


> +} IEC958Context;
> +

> +//TODO move to DSP

please do, we alraedy have bswap_buf there


[...]
> +static int spdif_header_dts(AVFormatContext *s, AVPacket *pkt)
> +{
> +    IEC958Context *ctx = s->priv_data;
> +    uint32_t syncword_dts = AV_RB32(pkt->data);
> +    int blocks;
> +
> +    switch (syncword_dts) {
> +    case DCA_MARKER_RAW_BE:
> +        blocks = (AV_RB16(pkt->data + 4) >> 2) & 0x7f;
> +        break;
> +    case DCA_MARKER_RAW_LE:
> +        blocks = (AV_RL16(pkt->data + 4) >> 2) & 0x7f;
> +        break;
> +    case DCA_MARKER_14B_BE:
> +        blocks =
> +            (((pkt->data[5] & 0x07) << 4) | ((pkt->data[6] & 0x3f) >> 2));
> +        break;
> +    case DCA_MARKER_14B_LE:
> +        blocks =
> +            (((pkt->data[4] & 0x07) << 4) | ((pkt->data[7] & 0x3f) >> 2));
> +        break;
> +    default:

> +        av_log(s, AV_LOG_ERROR, "bad DTS syncword\n");

that should print the syncword


> +        return -1;
> +    }
> +    blocks++;

> +    av_log(s, AV_LOG_DEBUG, "blocks=%i\n", blocks);

hmm


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

case  512 >> 5: ctx->data_type = IEC958_DTS1; break;
case 1024 >> 5: ctx->data_type = IEC958_DTS2; break;


[...]
> +static int spdif_header_mpeg(AVFormatContext *s, AVPacket *pkt)
> +{
> +    IEC958Context *ctx = s->priv_data;
> +    int version = (pkt->data[1] >> 3) & 3;
> +    int layer = 3 - ((pkt->data[1] >> 1) & 3);
> +    int extension = pkt->data[2] & 1;
> +
> +    if (layer == 3 || version == 1) {
> +        av_log(s, AV_LOG_ERROR, "Wrong MPEG file format\n");
> +        return -1;
> +    }

no layer 3 (mp3) ?

and you arent duplicating ff_mpegaudio_decode_header() are you?


[...]
> +static int spdif_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> +{
> +    IEC958Context *ctx = s->priv_data;
> +    int ret, padding;
> +
> +    ctx->pkt_size = FFALIGN(pkt->size, 2) << 3;
> +    ret = ctx->header_info(s, pkt);
> +    if (ret < 0)
> +        return -1;

is it needed to call this for each packet? that is, is it allowed to change?


> +
> +    padding = (ctx->pkt_offset - BURST_HEADER_SIZE - pkt->size) >> 1;
> +    if (padding < 0) {
> +        av_log(s, AV_LOG_ERROR, "bitrate is too high\n");
> +        return -1;
> +    }
> +
> +    put_le16(s->pb, SYNCWORD1);      //Pa
> +    put_le16(s->pb, SYNCWORD2);      //Pb
> +    put_le16(s->pb, ctx->data_type); //Pc
> +    put_le16(s->pb, ctx->pkt_size);  //Pd
> +
> +#if HAVE_BIGENDIAN
> +    put_buffer(s->pb, pkt->data, pkt->size & ~1);
> +#else

> +    if (ctx->buffer_size < pkt->size) {
> +        av_fast_malloc(&ctx->buffer, &ctx->buffer_size, pkt->size + FF_INPUT_BUFFER_PADDING_SIZE);
> +        if (!ctx->buffer)
> +            return AVERROR(ENOMEM);
> +    }

why the if() ?


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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20090817/aade6ff4/attachment.pgp>



More information about the ffmpeg-devel mailing list