[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