GNOME Bugzilla – Bug 769278
aacparse: a few fixes and improvements for LOAS parsing
Last modified: 2016-09-30 07:03:50 UTC
A bug in gst_aac_parse_latm_get_value (reading one fewer byte than needed), a bit more parsing of the AudioSpecificConfig structure, and keep track of the last parsed channels/rate fields so they can be used even if the element was not yet configured. Also some explanatory comments.
Created attachment 332284 [details] [review] aacparse: a few fixes and improvements for LOAS parsing
Review of attachment 332284 [details] [review]: Can you link here to some sample files that your patch helps with? Also this sounds like it should be a few separate commits instead of one huge, looks ok otherwise ::: gst/audioparsers/gstaacparse.c @@ +586,3 @@ + (void) extension_audio_object_type; + (void) ps; + (void) sbr; Not nice :)
I don't have a sample where this helps :) I found those things while trying to make some sample work, but it turns out it's not LOAS but some other obscure type of AAC. I'll split into several patches. The casts are because I added parsing of more stuff, but there's tons of other stuff after that, and so these few variables aren't actually used (yet), so GCC would moan about it, and, you know, -Werror is being its usual party pooper. I could remove that extra parsing code, since I don't even know if it works, due to the no-LOAS-file thing above, but it seemed like it was adding something incremental that would be useful later.
Created attachment 334767 [details] [review] strip unneeded slack space
Created attachment 334768 [details] [review] fix varlength integer reading
Created attachment 334769 [details] [review] improve rate/channel handling
Created attachment 334770 [details] [review] add comments in parsing code
Created attachment 334771 [details] [review] clearer logs
Created attachment 334772 [details] [review] more parsing
The one with the void casts is last, and can be omitted.
Comment on attachment 334769 [details] [review] improve rate/channel handling You probably want to reset these on FLUSH_STOP, PAUSED->READY, maybe new caps and other occasions.
Review of attachment 334772 [details] [review]: ::: gst/audioparsers/gstaacparse.c @@ +568,3 @@ + return FALSE; + + if (audio_object_type == 22) { How can we have audio_object_type == 5 || 29 above, and then here suddenly 22? Does get_audio_object_type() read it from a different place than where we got it before? @@ +586,3 @@ + (void) extension_audio_object_type; + (void) ps; + (void) sbr; I'd rather comment these out. It's relatively common to do these things, but it's actually undefined C behaviour AFAIU. The compiler would be correct to just optimize away the whole function, call system("rm -rf /") or anything else here ;)
Created attachment 334887 [details] [review] improve rate/channel handling
(In reply to Sebastian Dröge (slomo) from comment #12) > + if (audio_object_type == 22) { > > How can we have audio_object_type == 5 || 29 above, and then here suddenly > 22? Does get_audio_object_type() read it from a different place than where > we got it before? It reads from the bitstream every time, so it's a different place, yes. The spec for all that format here is fascinatingly complex (hence why a lot of it is left out). > @@ +586,3 @@ > + (void) extension_audio_object_type; > + (void) ps; > + (void) sbr; > > I'd rather comment these out. It's relatively common to do these things, but > it's actually undefined C behaviour AFAIU. The compiler would be correct to > just optimize away the whole function, call system("rm -rf /") or anything > else here ;) Casting a rvalue to void is undefined behavior ?
Review of attachment 334887 [details] [review]: ::: gst/audioparsers/gstaacparse.c @@ +1627,3 @@ + aacparse->last_parsed_sample_rate = 0; + } + return GST_ELEMENT_CLASS (parent_class)->change_state (element, transition); This is equivalent to just doing it in _start(), no?
(In reply to Vincent Penquerc'h from comment #14) > (In reply to Sebastian Dröge (slomo) from comment #12) > > > + if (audio_object_type == 22) { > > > > How can we have audio_object_type == 5 || 29 above, and then here suddenly > > 22? Does get_audio_object_type() read it from a different place than where > > we got it before? > > It reads from the bitstream every time, so it's a different place, yes. The > spec for all that format here is fascinatingly complex (hence why a lot of > it is left out). Ok then :) > > @@ +586,3 @@ > > + (void) extension_audio_object_type; > > + (void) ps; > > + (void) sbr; > > > > I'd rather comment these out. It's relatively common to do these things, but > > it's actually undefined C behaviour AFAIU. The compiler would be correct to > > just optimize away the whole function, call system("rm -rf /") or anything > > else here ;) > > Casting a rvalue to void is undefined behavior ? AFAIK yes, but feel free to ignore me
(In reply to Sebastian Dröge (slomo) from comment #15) > Review of attachment 334887 [details] [review] [review]: > > ::: gst/audioparsers/gstaacparse.c > @@ +1627,3 @@ > + aacparse->last_parsed_sample_rate = 0; > + } > + return GST_ELEMENT_CLASS (parent_class)->change_state (element, > transition); > > This is equivalent to just doing it in _start(), no? I don't know. baseparse state change doesn't call start directly, but it might well be a side effect.
(In reply to Sebastian Dröge (slomo) from comment #16) > > Casting a rvalue to void is undefined behavior ? > > AFAIK yes, but feel free to ignore me If you have a reference, I'd like to see that. I can't find confirmation after a cursory search and it seems unlikely.
Please use G_GNUC_UNUSED then instead of these void casts :)
(In reply to Vincent Penquerc'h from comment #18) > (In reply to Sebastian Dröge (slomo) from comment #16) > > > > Casting a rvalue to void is undefined behavior ? > > > > AFAIK yes, but feel free to ignore me > > If you have a reference, I'd like to see that. I can't find confirmation > after a cursory search and it seems unlikely. Found something in the C++ standard, which says you're right.
Created attachment 334906 [details] [review] more parsing
(In reply to Vincent Penquerc'h from comment #17) > (In reply to Sebastian Dröge (slomo) from comment #15) > > Review of attachment 334887 [details] [review] [review] [review]: > > > > ::: gst/audioparsers/gstaacparse.c > > @@ +1627,3 @@ > > + aacparse->last_parsed_sample_rate = 0; > > + } > > + return GST_ELEMENT_CLASS (parent_class)->change_state (element, > > transition); > > > > This is equivalent to just doing it in _start(), no? > > I don't know. baseparse state change doesn't call start directly, but it > might well be a side effect. My point was that start() is the perfect place for this. It's called upon pad activation, which is exactly the same in the end :) Can you move that there and remove the state change handler? And then merge
Created attachment 334910 [details] [review] improve rate/channel handling
commit c974df1c061157f79124ec84c3e410ed05d2fff7 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 5 09:39:33 2016 +0100 aacparse: parse a bit more of the humongous LOAS data https://bugzilla.gnome.org/show_bug.cgi?id=769278 commit e66ee5491c308dfbb6f82956c567d91977d223eb Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 5 09:39:08 2016 +0100 aacparse: make it clear when a potential LOAS frame is not one https://bugzilla.gnome.org/show_bug.cgi?id=769278 commit b0f20bacfd66d0c5d04861ec2846ebb9a76d2ad9 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 5 09:38:26 2016 +0100 aacparse: add a few comments to anchor parsing to the spec https://bugzilla.gnome.org/show_bug.cgi?id=769278 commit 559546dd3a3715150eac270751620fffa359eaea Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 5 09:37:02 2016 +0100 aacparse: improve channel/rate handling Keep track of the last parsed channels/rate fields so they can be used even if the element was not yet configured. https://bugzilla.gnome.org/show_bug.cgi?id=769278 commit 740749ac55f680ad61a328f86b595fa2837dd14c Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 5 09:35:53 2016 +0100 aacparse: fix varlength number reading as per spec https://bugzilla.gnome.org/show_bug.cgi?id=769278 commit 991e46ce421aa5e47c3d03d54bf9ad00e7c9a07a Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Sep 5 09:35:02 2016 +0100 aacparse: strip uneeded static arrays slack https://bugzilla.gnome.org/show_bug.cgi?id=769278