Opened 13 years ago

Closed 12 years ago

#372 closed defect (needs_more_info)

spdifenc does an output not correct

Reported by: naoya Owned by:
Priority: important Component: avformat
Version: git Keywords: spdifenc
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: yes

Description

I write SPDIF pass-through decoder for mplayer.
It using FFmpeg spdif muxer.
I found some problems in spdifenc.
I attach patch on this ticket.

  • following codec are tested.
  1. MPEG2 AAC
  2. AC3
  3. DTS
  • following codec are not tested.
  1. MPEG4 AAC
  2. MPEG AUDIO Layer1-3
  3. E-AC3
  4. DTS-HD
  5. TrueHD

Attachments (3)

spdifenc.patch (3.7 KB ) - added by naoya 13 years ago.
spdifenc.patch
spdifenc.patch2 (3.6 KB ) - added by naoya 13 years ago.
spdifenc.patch2
spdifenc3.patch (3.0 KB ) - added by naoya 13 years ago.

Download all attachments as: .zip

Change History (13)

by naoya, 13 years ago

Attachment: spdifenc.patch added

spdifenc.patch

comment:1 by Carl Eugen Hoyos, 13 years ago

Status: newopen

Could you explain in which cases your patch is needed?

And do I understand you correctly that you were able to test AAC (at least one variant)?

comment:2 by Anssi Hannula, 13 years ago

I guess the intent for the AC3 and AAC changes is to discard garbage at the end of AVPacket? I'm not sure if that should be handled, but I'm not against it. cehoyos, what do you think?

+    ctx->pkt_offset  = hdr.num_blocks * 256 << 2; 

Do you have samples where hdr.num_blocks is not 6? IEC 61937-3 mandates a repetition period of 6144 bytes for AC-3, and non-6 values here would produce a different repetition period. If you don't have any samples, I'd guess we should simply refuse to mux such AC-3 streams.

-    ctx->length_code = FFALIGN(pkt->size, 2) << 3; 

ctx->length_code is not set by every packetizer function, this causes 0 to be written for some codecs.

-    av_fast_malloc(&ctx->buffer, &ctx->buffer_size, ctx->out_bytes + FF_INPUT_BUFFER_PADDING_SIZE);
+        ctx->buffer = av_mallocz(ctx->out_bytes + FF_INPUT_BUFFER_PADDING_SIZE); 

Memory leak, no?

by naoya, 13 years ago

Attachment: spdifenc.patch2 added

spdifenc.patch2

in reply to:  1 comment:3 by naoya, 13 years ago

Replying to cehoyos:

And do I understand you correctly that you were able to test AAC (at least one variant)?

YES.
My AV-Receiver(ONKYO TX-SA608) support MPEG2 AAC.
It does not support MPEG4 AAC.

in reply to:  2 ; comment:4 by naoya, 13 years ago

Replying to anssi:

I guess the intent for the AC3 and AAC changes is to discard garbage at the end of AVPacket? I'm not sure if that should be handled, but I'm not against it. cehoyos, what do you think?

If AVPacket.size is greater than AVPacket.data size in it.(However, this is a problem of the parser)
Invalid IEC 61937 stream will out going.
Check byte lengh of AVPacket.data and use that value.

+    ctx->pkt_offset  = hdr.num_blocks * 256 << 2; 

Do you have samples where hdr.num_blocks is not 6? IEC 61937-3 mandates a repetition period of 6144 bytes for AC-3, and non-6 values here would produce a different repetition period. If you don't have any samples, I'd guess we should simply refuse to mux such AC-3 streams.

I also think so.
My DVD(only 10 samples) value is 6.

-    ctx->length_code = FFALIGN(pkt->size, 2) << 3; 

ctx->length_code is not set by every packetizer function, this causes 0 to be written for some codecs.

MPA function does not set ctx->length_code.
I revert that code back to the original.

-    av_fast_malloc(&ctx->buffer, &ctx->buffer_size, ctx->out_bytes + FF_INPUT_BUFFER_PADDING_SIZE);
+        ctx->buffer = av_mallocz(ctx->out_bytes + FF_INPUT_BUFFER_PADDING_SIZE); 

Memory leak, no?

This will leak. add free function.

Patch was updated.

in reply to:  4 ; comment:5 by Anssi Hannula, 13 years ago

Replying to naoya:

+#ifndef AC3_HEADER_SIZE

Is there a reason for the conditional here?

+ av_log(s, AV_LOG_ERROR, "Wrong AC3 file format\n");

I think better would be "Invalid AC3 header\n" or similar

+ av_log(s, AV_LOG_ERROR, "AC3 invalid num_blocks[%d]\n", hdr.num_blocks);

I don't think non-6 values here are "invalid", just unsupported by the muxer. So I'd go for "AC3 num_blocks[%d] not supported for IEC-61937" or something.

Also, the leak was not fixed. Why not just keep av_fast_malloc() as it was previously?

Regarding the replacement of av_freep() with av_free(). The documentation says that av_freep() is recommended instead, but indeed in this case that doesn't make much sense since the structure is discarded anyway. Cehoyos, do you know what is the convention here?

comment:6 by Carl Eugen Hoyos, 13 years ago

av_freep() should be used even if it makes no big difference.

by naoya, 13 years ago

Attachment: spdifenc3.patch added

in reply to:  5 ; comment:7 by naoya, 13 years ago

Replying to anssi:

Replying to naoya:

+#ifndef AC3_HEADER_SIZE

Is there a reason for the conditional here?

#ifndef is not needed.

+ av_log(s, AV_LOG_ERROR, "Wrong AC3 file format\n");

I think better would be "Invalid AC3 header\n" or similar

I agree.
By the same token, I think better would modify AAC&MPEG function.

+ av_log(s, AV_LOG_ERROR, "AC3 invalid num_blocks[%d]\n", hdr.num_blocks);

I don't think non-6 values here are "invalid", just unsupported by the muxer. So I'd go for "AC3 num_blocks[%d] not supported for IEC-61937" or something.

I agree.

Also, the leak was not fixed. Why not just keep av_fast_malloc() as it was previously?

I had mistake.
av_fast_malloc() has no problem.

Regarding the replacement of av_freep() with av_free(). The documentation says that av_freep() is recommended instead, but indeed in this case that doesn't make much sense since the structure is discarded anyway. Cehoyos, do you know what is the convention here?

I understand.

patch updated.

in reply to:  7 comment:8 by jbr, 13 years ago

Replying to naoya:

+ av_log(s, AV_LOG_ERROR, "AC3 invalid num_blocks[%d]\n", hdr.num_blocks);

I don't think non-6 values here are "invalid", just unsupported by the muxer. So I'd go for "AC3 num_blocks[%d] not supported for IEC-61937" or something.

I agree.

num_blocks will never be anything other than 6 for AC-3. Only E-AC-3 can have that, but a different function is used for E-AC-3. and if you actually want to check that it's AC-3 and not E-AC-3, you should check hdr.bitstream_id.

comment:9 by Anssi Hannula, 12 years ago

I'm sorry for the very long delay, I had forgotten this one.

Can you tell me why do you need this?

I'm not sure it is the muxer's job to drop trailing garbage, but I'm willing to have it like that if you can provide a good reason for it. I.e. I want to know why would there be trailing garbage in the first place?

comment:10 by Michael Niedermayer, 12 years ago

Resolution: needs_more_info
Status: openclosed

No reply to the questions 5 and 11 months ago, thus closing this. Please reopen when the questions/comments have been taken care of or there are any other updates that merrits reopening.
Thanks!

Note: See TracTickets for help on using tickets.