Opened 4 years ago

Closed 3 years ago

#4449 closed defect (needs_more_info)

Error in mpegaudio_parser.c

Reported by: Dan203 Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords: mpegaudio
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

There is a logic error in the mpegaudio_parser.c code. This line...

int header_threshold = avctx->codec_id != AV_CODEC_ID_NONE && avctx->codec_id != codec_id;

should be...

int header_threshold = avctx->codec_id != AV_CODEC_ID_NONE && avctx->codec_id == codec_id;

The current code causes the parser to skip over setting the params if the codec_id set by the demuxer is different then the one detected by the parser. But it should skip setting them when they are the same not when they are different.

Change History (6)

comment:1 Changed 3 years ago by michael

See: https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2015-April/171367.html
Your analysis is not correct
there is nothing that can be done without a source media file showing the problem aka a testcase

comment:2 follow-up: Changed 3 years ago by Dan203

Any file with MP2 audio should be sufficient as a test case. After the call to avformat_find_stream_info the audio will not be detected as MP2. Only after the first call to av_read_frame does it get filled in correctly. I changed the parser as I described and it works correctly.

comment:3 Changed 3 years ago by cehoyos

  • Keywords mpegaudio added; mpegaudio_parser removed

If you want to report an issue with FFmpeg, please provide a sample input file and explain how the issue can be reproduced.
If you have a patch that fixes an issue with current FFmpeg, please send your patch (made with git format-patch to the development mailing list, patches are only discussed there.

comment:4 in reply to: ↑ 2 Changed 3 years ago by heleppkes

Replying to Dan203:

Any file with MP2 audio should be sufficient as a test case. After the call to avformat_find_stream_info the audio will not be detected as MP2. Only after the first call to av_read_frame does it get filled in correctly. I changed the parser as I described and it works correctly.

"Working correctly" is a rather broad generalization. It works correctly for your file. Does it work correctly for every file? Probably not.

By looking at that line alone, its quite obvious that your suggested change is incorrect, because the first condition would always be true if the second condition is also true, therefor it is redundant - which is a clear indicator that the original code was what the author intended.

So instead, I would greatly recommend to provide a file with which the problem can be reproduced, and then we can see what can be done about it.

comment:5 Changed 3 years ago by Dan203

Perhaps you're right that my change is not the way to fix it. But the code is not working properly even if it is the way the author intended. It appears to me that maybe he was trying to ensure that at least 2 headers were found before changing the codec ID. The problem is that when you call avformat_find_stream_info it only calls mpegaudio_parse once, so the author's code...

int header_threshold = avctx->codec_id != AV_CODEC_ID_NONE && avctx->codec_id != codec_id;

evaluates to 1 because avctx->codec_id is AV_CODEC_ID_MP3 and codec_id is AV_CODEC_ID_MP2. It then increments s->header_count from 0 to 1 and then hits this block...

if (s->header_count > header_threshold) {

which fails because both s->header_count and header_threshold are now 1. s->header_count does not increase to greater then 1 until after the first call to av_read_frame which returns an frame from that stream.

What this means is that there is no way for the user (me) to pre-test a file to determine the codecs of all the streams. This worked fine in an older version we were using which didn't have the header_threshold test and simply did...

if (s->header_count > 0) {

I'm not sure why the change was made but it seems to break the ability of avformat_find_stream_info to properly determine the codec_id of MP2 streams.

comment:6 Changed 3 years ago by cehoyos

  • Resolution set to needs_more_info
  • Status changed from new to closed

Please reopen this ticket if you can provide a sample and if you can explain how to reproduce the issue.

Note: See TracTickets for help on using tickets.