GNOME Bugzilla – Bug 724638
aacparse : Missing resilience when no audio frame is found
Last modified: 2014-09-03 07:46:08 UTC
Hi, I see an issue in aacparse which makes baseparse to go to an assert Here are the sequence of events 1. The input stream has some errors. Not fatal error though. VLC recovers and continues to play 2. gst_aac_parse_check_adts_frame() sees no input(possibly corrupted frame). framesize variable is set to zero but return value is TRUE 3. gst_base_parse_finish_frame() function is called with size=0 and frame->out_buffer set to NULL. This makes aacparse to go to assert and never recover I just found a workaround by making hardcoding ret of gst_aac_parse_check_adts_frame() to FALSE if framesize is zero. This works to some extent but not sure if it is right Alternatively, G_DISABLE_CHECKS while building glib might avoid the assert but I'm not sure if it is right to do that I have uploaded my sample file using which it can be easily reproduced with following pipeline gst-launch-1.0 filesrc location=aacparse_issue.ts ! tsdemux ! aacparse ! fakesink Regards, Jagadish
The sample file can be found at https://drive.google.com/file/d/0B9x__4fSqnlIcV9tX2xDZW1OX2M/edit?usp=sharing
The problem seems a bit more involved and somewhat racy. I stop hitting the assert at GST_DEBUG=*:2 and beyond. In these cases I get this instead (same pipeline): Pipeline is PREROLLING ... Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstSystemClock 0:00:00.055622733 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 11, stream 3 0:00:00.062322623 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 1, stream 9 0:00:00.066896719 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 11, stream 3 0:00:00.068401171 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 0, stream 8 0:00:00.069449472 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 15, stream 8 0:00:00.069544452 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 13, stream 11 0:00:00.073552278 4588 0x262ead0 WARN tsdemux tsdemux.c:1531:gst_ts_demux_queue_data: CONTINUITY: Mismatch packet 14, stream 6 0:00:00.075468951 4588 0x262ead0 WARN default codec-utils.c:83:gst_codec_utils_aac_get_sample_rate_from_index: Invalid sample rate index 14 Got EOS from element "pipeline0". Execution ended after 0:00:00.023885165 Setting pipeline to PAUSED ... Setting pipeline to READY ... Setting pipeline to NULL ... Freeing pipeline ... I will take another look once I get some time (if no one beats me to it). Thanks for your report.
Created attachment 269641 [details] [review] Make sure we have enough data at header parsing This patch by itself solves the bug for me
Created attachment 269644 [details] [review] Add some more sanity checks This two extra sanity checks can be added to the code to be more strict about the ADTS header's validity. Reference: http://wiki.multimedia.cx/index.php?title=ADTS
Review of attachment 269641 [details] [review]: ::: gst/audioparsers/gstaacparse.c @@ +390,3 @@ *needed_data = 0; + if (G_UNLIKELY (avail < 6)) { This should be moved into the if below as it only requires 6 bytes if it is going to call _adts_get_frame_len. So I'd suggest keeping the check for 2 bytes that is enough for the if below and moving the check for 6 bytes inside the if.
Hi Thiago, thanks for your review. If I proceed that way I will have to leave the > 2 check in place just for checking for > 6 bellow the main if block. Which doesn't make a lot of sense considering the function should do nothing but return FALSE if those conditions aren't met. I actually considered dropping the whole main if and have these few checks on top but as that required to reindent the whole function for no procedural reason I opted out of it. Can you take a look again and comment please?
Review of attachment 269641 [details] [review]: Ok, please go ahead and commit. This seems to be what the loas check function does, too.
Comment on attachment 269641 [details] [review] Make sure we have enough data at header parsing >+ if (G_UNLIKELY (avail < 6)) { >+ *needed_data = 6 - avail; > return FALSE; >+ } Are you sure that's not supposed to be *needed_data = 6 ?
Hi, I tested with Reynadlo's patch and it seems to work fine for the case that I shared. Also, for few other non erroneous cases(just wanted to confirm it doesn't break anything) Regards, Jagadish
(In reply to comment #8) > (From update of attachment 269641 [details] [review]) > >+ if (G_UNLIKELY (avail < 6)) { > >+ *needed_data = 6 - avail; > > return FALSE; > >+ } > > Are you sure that's not supposed to be *needed_data = 6 ? Hi Tim, thanks for your comment. Yeah, it is. I got confused by the function's doc header: ..how much _more_ data is needed in the next round.. but after your comment, realized further up in the calling tree this boils down to: gst_base_parse_set_min_frame_size(..., needed_data)... This also made me reconsider Thiago's comment for a different reason: I shouldn't be setting needed_data if there isn't at least some clue this might be an ADTS frame. Patches updated & tested
Created attachment 269703 [details] [review] Make sure we have enough data at header parsing
Created attachment 269705 [details] [review] Add some more sanity checks
(In reply to comment #9) > Hi, > > I tested with Reynadlo's patch and it seems to work fine for the case that I > shared. Also, for few other non erroneous cases(just wanted to confirm it > doesn't break anything) Thanks. Can you test with the new patch set and confirm everything works for you?
Hello Reynaldo, I have also seen similar issue in my test case. I applied your latest patch and verified it for erroneous and non-erroneous test case. It looks fine. Thanks for your quick help.
(In reply to comment #14) > Hello Reynaldo, I have also seen similar issue in my test case. I applied your > latest patch and verified it for erroneous and non-erroneous test case. It > looks fine. Thanks for your quick help. No problem. Thanks for testing.
Hi, I did test with this patch. Seems ok. Tested with erroneous and normal ADTS cases. I couldn't test with LOAS cases since I do not have the content :( Regards, Jagadish
Comment on attachment 269705 [details] [review] Add some more sanity checks Please hold off on this one for now.
Comment on attachment 269705 [details] [review] Add some more sanity checks >+ gboolean crc_absent; Turn that into a crc_size variable instead. >+ /* Sampling frequency test */ >+ if (G_UNLIKELY ((data[2] & 0x3C) >> 2 == 15)) { >+ GST_WARNING ("Invalid ADTS header: sampling frequency can't be 15"); Drop the GST_WARNING here please, seems a bit extreme just because we're not in sync for some reason. Not sure we need a logging statement here at all. >+ if ((*framesize < 9 && !crc_absent) || (*framesize < 7 && crc_absent)) { >+ GST_WARNING ("Invalid ADTS header: framesize %d too small (HAS CRC: %s)", >+ *framesize, crc_absent ? "No" : "Yes"); >+ return FALSE; >+ } Same as above, please get rid of the GST_WARNING (or demote to DEBUG if you must keep it, but it's really just noise imho).
Created attachment 269933 [details] [review] Add some more sanity checks Dropped the logging and moved to a crc_size var to simplify the minimum frame size test's expression.
Comment on attachment 269933 [details] [review] Add some more sanity checks Thanks! (if you could quickly add a newline after the 'guint crc_size;' declaration too before pushing, that'd be appreciated, looks like gst-indent doesn't take care of that)
Done. Thanks for your review/testing time guys.
Comment on attachment 269933 [details] [review] Add some more sanity checks Added the spacing nit spotted by Tim
Hi Reynaldo, Is this patch pushed with 1.4.1 version??
It's in 1.2.4 and newer (i.e. all 1.4 releases)