[FFmpeg-devel] IEC61937 compatible muxer

Bartlomiej Wolowiec bartek.wolowiec
Mon Aug 17 11:25:56 CEST 2009


Monday 17 August 2009 02:43:53 Michael Niedermayer napisa?(a):
> 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?

Yes, it does not appear...

> > +    int pkt_offset;                 ///< Repetition period of data burst
> > in bytes
>
> is it just me or is the doxy unclear?

it's like number of bytes between two next data bursts (packets, which have 
header and burst payload (encapsuled frame)). Should i change comments, or 
maybe add at begining of file some explanation of terminology?

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

i thought about it, but there is one little problem - dsputil_init() gets
 pointer to AVCodecContext... Any idea how should i use it in libavformat ?

> [...]
>
> > +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?

it's not exactly like that... layer+1 is real mpeg layer (as you can see at 
comment at mpeg_pkt_offset and mpeg_data_type).
i think i'm not duplicating ff_mpegaudio_decode_header() - it doesn't save 
mpeg25 and extension bit, which i use.

> [...]
>
> > +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?

I didn't find any information about constants of it, so i think it may 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() ?
-- 
Bartlomiej Wolowiec



More information about the ffmpeg-devel mailing list