IEC61937 compatible muxer
Hi, Here is IEC61937 compatible muxer. It can be used for sending compressed data over S/PDIF link. It's currently not very useful, but can be tested by: ./ffmpeg -y -i test_48khz.dts -acodec copy -f spdif test.spdif && ./ffmpeg -ar 48000 -f s16le -ac 2 -i test.spdif -f alsa plughw:0 -- Bartlomiej Wolowiec
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.
+ switch (syncword_dts) { + case DCA_MARKER_RAW_BE: + blocks = + (((pkt->data[4] & 0x01) << 6) | (pkt->data[5] >> 2)) + 1; + break; + case DCA_MARKER_RAW_LE: + blocks = + (((pkt->data[5] & 0x01) << 6) | (pkt->data[4] >> 2)) + 1; + break; + case DCA_MARKER_14B_BE: + blocks = + (((pkt->data[5] & 0x07) << 4) | ((pkt->data[6] & 0x3f) >> 2)) + 1; + break; + case DCA_MARKER_14B_LE: + blocks = + (((pkt->data[4] & 0x07) << 4) | ((pkt->data[7] & 0x3f) >> 2)) + 1; + break; + default: + av_log(s, AV_LOG_ERROR, "bad DTS syncword\n"); + return -1; + }
Hm, the + 1 could be moved outside the switch, and the first two are the same as ((AV_RB16(pkt->data + 4) >> 2) & 0x7f) + 1; ((AV_RL16(pkt->data + 4) >> 2) & 0x7f) + 1;
+ 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)
+ 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).
+ NULL, + //.flags=
Both seem pointless.
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. -- Bartlomiej Wolowiec
On Sat, Aug 08, 2009 at 12:22:45AM +0200, Bartlomiej Wolowiec wrote:
http://www.ffmpeg.org/developer.html#SEC7 http://www.ffmpeg.org/developer.html#SEC8 Specifically this is missing a documentation update and there are still some opportunities for vertically aligning code for better readability. Diego
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
Monday 10 August 2009 13:38:56 Michael Niedermayer napisa?(a):
[...]
+ ? ?{ + ? ? ? ?//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
but how should i get large buffer ? call av_malloc/av_free every time ? or call av_malloc only when old buffer has not enough size ? -- Bartlomiej Wolowiec
On Wed, Aug 12, 2009 at 09:31:31PM +0200, Bartlomiej Wolowiec wrote:
Monday 10 August 2009 13:38:56 Michael Niedermayer napisa?(a):
[...]
+ ? ?{ + ? ? ? ?//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
but how should i get large buffer ? call av_malloc/av_free every time ? or call av_malloc only when old buffer has not enough size ?
try av_fast_realloc() [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Avoid a single point of failure, be that a person or equipment.
On Wed, Aug 12, 2009 at 09:51:06PM +0200, Michael Niedermayer wrote:
On Wed, Aug 12, 2009 at 09:31:31PM +0200, Bartlomiej Wolowiec wrote:
Monday 10 August 2009 13:38:56 Michael Niedermayer napisa?(a):
[...]
+ ? ?{ + ? ? ? ?//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
but how should i get large buffer ? call av_malloc/av_free every time ? or call av_malloc only when old buffer has not enough size ?
try av_fast_realloc()
Shouldn't that be av_fast_malloc ? I think it doesn't need the previous data...
On Wed, Aug 12, 2009 at 10:23:14PM +0200, Reimar D?ffinger wrote:
On Wed, Aug 12, 2009 at 09:51:06PM +0200, Michael Niedermayer wrote:
On Wed, Aug 12, 2009 at 09:31:31PM +0200, Bartlomiej Wolowiec wrote:
Monday 10 August 2009 13:38:56 Michael Niedermayer napisa?(a):
[...]
+ ? ?{ + ? ? ? ?//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
but how should i get large buffer ? call av_malloc/av_free every time ? or call av_malloc only when old buffer has not enough size ?
try av_fast_realloc()
Shouldn't that be av_fast_malloc ? I think it doesn't need the previous data...
indeed ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
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
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
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
On Mon, Aug 17, 2009 at 11:25:56AM +0200, Bartlomiej Wolowiec wrote:
Monday 17 August 2009 02:43:53 Michael Niedermayer napisa?(a): [..]
+ 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?
i guess both are a good idea
+ 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 ?
i have a few dsputil could be split into a AVCodecContext specific context and a non specific one, the later could be shared amongth all "users". though i guess that could be done after the patch is commited [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better.
Tuesday 18 August 2009 01:31:58 Michael Niedermayer napisa?(a):
On Mon, Aug 17, 2009 at 11:25:56AM +0200, Bartlomiej Wolowiec wrote:
Monday 17 August 2009 02:43:53 Michael Niedermayer napisa?(a):
[..]
+ 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?
i guess both are a good idea
+ 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 ?
i have a few dsputil could be split into a AVCodecContext specific context and a non specific one, the later could be shared amongth all "users".
though i guess that could be done after the patch is commited
[...]
Ok, i attach improved patch. -- Bartlomiej Wolowiec
On Tue, Aug 18, 2009 at 12:03:32PM +0200, Bartlomiej Wolowiec wrote:
Tuesday 18 August 2009 01:31:58 Michael Niedermayer napisa?(a):
On Mon, Aug 17, 2009 at 11:25:56AM +0200, Bartlomiej Wolowiec wrote:
Monday 17 August 2009 02:43:53 Michael Niedermayer napisa?(a):
[..]
+ 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?
i guess both are a good idea
+ 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 ?
i have a few dsputil could be split into a AVCodecContext specific context and a non specific one, the later could be shared amongth all "users".
though i guess that could be done after the patch is commited
[...]
Ok, i attach improved patch.
-- Bartlomiej Wolowiec
Changelog | 1 doc/general.texi | 1 libavformat/Makefile | 1 libavformat/allformats.c | 1 libavformat/avformat.h | 2 libavformat/spdif.c | 310 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 315 insertions(+), 1 deletion(-) 5e1050033d862e1e5adcfc1cb8019129cc27a1cf patch
looks ok to me, given it works, has been tested and you add regression tests [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
Tuesday 18 August 2009 14:20:04 Michael Niedermayer napisa?(a):
Changelog | 1 doc/general.texi | 1 libavformat/Makefile | 1 libavformat/allformats.c | 1 libavformat/avformat.h | 2 libavformat/spdif.c | 310 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 315 insertions(+), 1 deletion(-) 5e1050033d862e1e5adcfc1cb8019129cc27a1cf patch
looks ok to me, given it works, has been tested and you add regression tests
[...]
I made some corrections suggested by Diego. Yes, this code is working (not all MPEG options are tested, but they should work), so should i commit this patch and make next commit with regression tests or commit all at once ? -- Bartlomiej Wolowiec
On Wed, Aug 19, 2009 at 10:59:04AM +0200, Bartlomiej Wolowiec wrote:
Tuesday 18 August 2009 14:20:04 Michael Niedermayer napisa?(a):
Changelog | 1 doc/general.texi | 1 libavformat/Makefile | 1 libavformat/allformats.c | 1 libavformat/avformat.h | 2 libavformat/spdif.c | 310 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 315 insertions(+), 1 deletion(-) 5e1050033d862e1e5adcfc1cb8019129cc27a1cf patch
looks ok to me, given it works, has been tested and you add regression tests
[...]
I made some corrections suggested by Diego. Yes, this code is working (not all MPEG options are tested, but they should work), so should i commit this patch and make next commit with regression tests or commit all at once ?
iam slightly in favor of 2 commits but thats very very slightly only [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
Wednesday 19 August 2009 12:08:10 Michael Niedermayer napisa?(a):
On Wed, Aug 19, 2009 at 10:59:04AM +0200, Bartlomiej Wolowiec wrote:
Tuesday 18 August 2009 14:20:04 Michael Niedermayer napisa?(a):
Changelog | 1 doc/general.texi | 1 libavformat/Makefile | 1 libavformat/allformats.c | 1 libavformat/avformat.h | 2 libavformat/spdif.c | 310 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 315 insertions(+), 1 deletion(-) 5e1050033d862e1e5adcfc1cb8019129cc27a1cf patch
looks ok to me, given it works, has been tested and you add regression tests
[...]
I made some corrections suggested by Diego. Yes, this code is working (not all MPEG options are tested, but they should work), so should i commit this patch and make next commit with regression tests or commit all at once ?
iam slightly in favor of 2 commits but thats very very slightly only
[...]
Hi, Patch applied. I thought about regression tests and I don't know what should i exactly do... Should there be used FFmpegs encoders (ac3, mp3...) to make right audio format and then save with spdif format or something else? -- Bartlomiej Wolowiec
Bartlomiej Wolowiec wrote:
Hi, Patch applied. I thought about regression tests and I don't know what should i exactly do... Should there be used FFmpegs encoders (ac3, mp3...) to make right audio format and then save with spdif format or something else?
To start with. Use the ac3 encoder and mux into spdif. Then use that file as reference. Fate tests with the other codes would be nice also. MvH Benjamin Larsson
On Tue, Aug 18, 2009 at 12:03:32PM +0200, Bartlomiej Wolowiec wrote:
Ok, i attach improved patch.
Please cut down your quotes, thank you.
--- libavformat/spdif.c (wersja 0) +++ libavformat/spdif.c (wersja 0) @@ -0,0 +1,310 @@ +/** + * @file libavformat/spdif.c + * IEC-61937 encapsulation of various formats, used at S/PDIF
s/at/with/ or s/at/by/ I think.
+typedef struct IEC958Context { + enum IEC958DataType data_type; ///< Burst info - reference to type of payload of the data-burst + int pkt_size; ///< Length code in bits + int pkt_offset; ///< Repetition period of data burst in bytes
data burst repetition period in bytes
+ uint8_t *buffer; ///< Allocated buffer, used for swap bytes + int buffer_size; ///< Size of allocated buffer
allocated buffer size nit: All of these comments should not be capitalized.
+ /// Function, which generates codec dependent header information (sets data_type and data_offset)
Please break overly long lines where easily possible.
+ for (; i < w; i++) { + dst[i + 0] = bswap_16(src[i + 0]); + }
useless {}
+ int version = (pkt->data[1] >> 3) & 3; + int layer = 3 - ((pkt->data[1] >> 1) & 3);
align
+ NULL_IF_CONFIG_SMALL("IEC958 (IEC-61937)"),
Maybe you could mention S/PDIF somewhere in there. Diego
Bartlomiej Wolowiec <bartek.wolowiec <at> gmail.com> writes:
Here is IEC61937 compatible muxer. It can be used for sending compressed data over S/PDIF link. It's currently not very useful, but can be tested by: ./ffmpeg -y -i test_48khz.dts -acodec copy -f spdif test.spdif && ./ffmpeg -ar 48000 -f s16le -ac 2 -i test.spdif -f alsa plughw:0
Did anybody ever test this (successfully)? With the exact command line (and a dts and an ac3 sample), I get "av_interleaved_write_frame(): Error while opening file" both with latest svn and {2009-08-22}. mplayer -ac hwac3 and -ac hwdts both work fine. (And issue1735 might address the same problem.) Carl Eugen
On Tue, 9 Feb 2010 10:05:17 +0000 (UTC), Carl Eugen Hoyos wrote:
Bartlomiej Wolowiec <bartek.wolowiec <at> gmail.com> writes:
[4 quoted lines suppressed]
Did anybody ever test this (successfully)? With the exact command line (and a dts and an ac3 sample), I get "av_interleaved_write_frame(): Error while opening file" both with latest svn and {2009-08-22}. mplayer -ac hwac3 and -ac hwdts both work fine. (And issue1735 might address the same problem.)
Carl Eugen
Not through ffmpeg program, but we are using the muxer in xbmc at the moment. There is one odd quirk in it thou. The write header function writes a bunch of zeroes to the stream before the data. The way we use it is writeheader/writedata/writetrailer for each decode packet (suspect this might not be correct), which prefixes each packet which a bunch of zeroes. Joakim
Carl Eugen Hoyos <cehoyos <at> ag.or.at> writes:
Bartlomiej Wolowiec <bartek.wolowiec <at> gmail.com> writes:
Here is IEC61937 compatible muxer. It can be used for sending compressed data over S/PDIF link. It's currently not very useful, but can be tested by: ./ffmpeg -y -i test_48khz.dts -acodec copy -f spdif test.spdif && ./ffmpeg -ar 48000 -f s16le -ac 2 -i test.spdif -f alsa plughw:0
Did anybody ever test this (successfully)? With the exact command line (and a dts and an ac3 sample), I get "av_interleaved_write_frame(): Error while opening file" both with latest svn and {2009-08-22}. mplayer -ac hwac3 and -ac hwdts both work fine. (And issue1735 might address the same problem.)
Output has changed now (but seems unaffected by r22814 (which changed the content of test.spdif): ./ffmpeg -ar 48000 -f s16le -ac 2 -i test.spdif -f alsa plughw:0 FFmpeg version SVN-r22852, Copyright (c) 2000-2010 the FFmpeg developers built on Apr 12 2010 16:46:58 with gcc 4.4.3 configuration: --cc=/usr/local/gcc-4.4.3/bin/gcc libavutil 50.14. 0 / 50.14. 0 libavcodec 52.66. 0 / 52.66. 0 libavformat 52.61. 0 / 52.61. 0 libavdevice 52. 2. 0 / 52. 2. 0 libswscale 0.10. 0 / 0.10. 0 [s16le @ 0x1110420]Estimating duration from bitrate, this may be inaccurate Input #0, s16le, from 'test2.spdif': Duration: N/A, start: 0.000000, bitrate: N/A Stream #0.0: Audio: pcm_s16le, 48000 Hz, 2 channels, s16, 1536 kb/s Output #0, alsa, to 'plughw:0': Metadata: encoder : Lavf52.61.0 Stream #0.0: Audio: pcm_s16le, 48000 Hz, 2 channels, s16, 1536 kb/s Stream mapping: Stream #0.0 -> #0.0 Press [q] to stop encoding av_interleaved_write_frame(): Resource temporarily unavailable
Bartlomiej Wolowiec <bartek.wolowiec <at> gmail.com> writes:
Here is IEC61937 compatible muxer. It can be used for sending compressed data over S/PDIF link. It's currently not very useful, but can be tested by: ./ffmpeg -y -i test_48khz.dts -acodec copy -f spdif test.spdif && ./ffmpeg -ar 48000 -f s16le -ac 2 -i test.spdif -f alsa plughw:0
I finally successfully tested playing test.spdif with aplay -Dplughw:1,3 -t raw -r 48000 -f S16_LE -c 2 test.spdif (1,3 is hdmi, it also works with -Diec958) Note that -c 2 is always needed! ffmpeg -f alsa always shows this error for me if I use the same plughw as above: av_interleaved_write_frame(): Resource temporarily unavailable Does anybody have suggestions on how spdif_header_eac3() should look like? Using spdif_header_ac3() leads to silence for eac3 (with the receiver showing "Dolby Digital" so I guess we first need IEC958_EAC3). I would happily test guesses. Carl Eugen
participants (7)
-
banan＠ludd.ltu.se -
bartek.wolowiec＠gmail.com -
cehoyos＠ag.or.at -
diego＠biurrun.de -
elupus＠ecce.se -
michaelni＠gmx.at -
Reimar.Doeffinger＠gmx.de