GNOME Bugzilla – Bug 686595
[mpg123] misc improvements and fixes
Last modified: 2012-10-24 12:45:15 UTC
Created attachment 226945 [details] [review] mpg123: collection of multiple minor fixes and comment - The last MP3 frame wasn't being pushed when base class was draining - Made sure mpg123 cannot ever use its (crude) internal resampler - Disabled mpg123 stderr output - Improved log output - Adjusted log level in various places - Replaced most GST_ELEMENT_ERROR calls with alternatives - Replaced mpg123decoder->handle != NULL checks with asserts - Some comment lines were overly long - Added gtkdoc block and related parts in -sections.txt and -docs.sgml - Added test to check/tests/elements/
Thanks, but perhaps you could break things up in smaller pieces next time (not 20, but perhaps also not one megapatch). Now, if I may nitpick a little: - set_format: perhaps "input_caps", but "upstream_caps" is not a term that's usually used in such a context. - for comments we prefer each line to have a '*' - an mpg123_open*() error is more a LIBRARY, INIT error than a RESOURCE, OPEN_READ (that's more for a device like a CD drive or so) - TRACE log level is for very frequently recurring things that add very little info in most cases (e.g. refcounting changes). Things in ::start/stop functions should be INFO or DEBUG - the "nothing was decoded" msg should probably be DEBUG rather than TRACE too, for example (not recurring during normal operation); and perhaps have sth added that it's nor something to be alarmed about, like 'need more frames' or 'need more data' or so - if you allocate a buffer for num_decoded_bytes and you get a buffer back, I don't think you need to check after the map call that you really have as many bytes as you allocated... (neither less nor more). - in handle_frame, why do you get and map the GstMemory instead of the buffer? - if gst_audio_decoder_set_output_format() fails, you should probably just return FLOW_NOT_NEGOTIATED, and maybe log something to the debug log with GST_WARNING or so (and not post an error message) - not sure about the error stuff, ideally the decoder would just ignore anything that's not absolutely fatal and try to resync (and ignore upstream caps if in doubt..) - perhaps the test data for the unit test could be dumped into files in tests/files/ ? Makes the code easier to peruse..
(In reply to comment #1) I split up the patches and addressed most issues you mentioned above. > - not sure about the error stuff, ideally the > decoder would just ignore anything that's > not absolutely fatal and try to resync > (and ignore upstream caps if in doubt..) 3 element_error calls are still present. One occurs when starting to decode fails, one when reopening the decoder fails ( while trying to flush), and one in case there is a problem with the input caps. I am unsure about the latter case, since it indicates a serious problem with the upstream mpeg parser. Also, I don't really know how to recover from that yet. I will see if a sane recovery is possible.
Created attachment 227129 [details] [review] cleaned up comments, formatting, and logging
Created attachment 227130 [details] [review] fixed bug with last frame, disabled internal resampler & chatter
Created attachment 227131 [details] [review] added gtkdoc section
Created attachment 227132 [details] [review] added test
Regarding the error handling: right, I don't know the mpg123 API, I was just saying how it should work ideally. I was surprised the API required you to pass in any details. And it is of course not entirely inconceivable that the parser messes up, and finds a bad frame sync (with bad caps) before getting back on a 'good' sync. I don't know if it's a problem in practice, was just pointing it out in case you had any ideas how to handle it.
commit cf1f4871d7e39a926a00c9b22532a31204670999 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Wed Oct 24 13:41:00 2012 +0100 tests: fix up mpg123 test a little - dist input files - fix sample leak - simplify check for elements - only run mpg123 test if mpg123 is available and selected - fix build in uninstalled setup https://bugzilla.gnome.org/show_bug.cgi?id=686595 commit 92118c0b11abf16bb1c3728f0340bf3202a0898e Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Wed Oct 24 12:30:10 2012 +0200 tets: add unit test for mpg123audiodec https://bugzilla.gnome.org/show_bug.cgi?id=686595 commit a84677a7a0eba120f087eac4e487ba4f744ed5cc Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Wed Oct 24 00:36:42 2012 +0200 mpg123: added gtkdoc section https://bugzilla.gnome.org/show_bug.cgi?id=686595 commit 8ba422839a3bd602a42f01140ca820e6b752055b Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Wed Oct 24 00:22:05 2012 +0200 mpg123: fixed bug with last frame, disabled internal resampler & chatter * The last MP3 frame wasn't being pushed when base class was draining * Made sure mpg123 cannot ever use its (crude) internal resampler * Disabled mpg123 stderr output https://bugzilla.gnome.org/show_bug.cgi?id=686595 commit 2de930689c57291c12f2ea5ec9af984eccf6af4e Author: Carlos Rafael Giani <dv@pseudoterminal.org> Date: Wed Oct 24 00:21:45 2012 +0200 mpg123: cleaned up comments, formatting, and logging lines also replaced mpg123decoder->handle != NULL checks with asserts https://bugzilla.gnome.org/show_bug.cgi?id=686595