GNOME Bugzilla – Bug 709180
mpegts: Return GstMpegTsDescriptor in mpegts_get_descriptor_from_*
Last modified: 2013-10-11 08:40:30 UTC
Created attachment 256178 [details] [review] Patch v1 Patch on master to correct a FIXME. The functions mpegts_get_descriptor_from_program and mpegts_get_descriptor_from_stream should return the GstMpegTsDescriptor instead of a pointer to descriptor data. I have touched the parts that uses the functions, but I don't know if the preferred way is to pass desc->data to the macros, or if the macros should do the ->data?
The part in mpegtsbase.[ch] is fine. The usage is a bit more tricky. *Ideally* we should be using mpeg-ts-lib functions, but the one for languages is a bit heavy and we don't have one for ac3 descriptor. For language, maybe we could have a one-shot variant: gboolean gst_mpegts_descriptor_parse_iso_639_language_idx (const GstMpegTsDescriptor *descriptor, guint idx, gchar *lang[4], GstMpegTsIso639AudioType *audio_type); idx: index in the table (if you ask for something bigger than the size of the descriptor the method returns FALSE and you can stop iterating) lang: output for the language code (NULL-terminated) for that index audio_type: output for the audio_type for that index This would allow doing while (gst_mpegts_descriptor_parse_iso_639_language_idx(desc, i, &lang, &type)) { // do something i++; } For the AC3 one I'm not 100% sure what the method should look like (The description of that descriptor is in ATSC A/52 specifications btw).
Created attachment 256598 [details] [review] Patch v2 Reworked the patch a bit. Split up into 3 commits. The first commit introduces new ISO 639 parsing functions in mpegts-lib: A function to get a specific table id, as mentioned in the previous comment. A function to get the number of languages in the descriptor. The second commit does the change in return type for the mpegts_get_descriptor_from_* functions. This commit uses the descriptors in the old non-ideal ways in order to by able to compile. The third commit changes the use of the language descriptor, so that the new mpegts-lib functions are used. Seems like a bit of thought is needed regarding the AC3 parsing, as this descriptor holds quite a lot of information. Maybe we can work on the current patch set as a step in the right direction?
Looks good. Can you make 3 different git patches ? Make sure the commit messages are sensible ("tsdemux: ...." "mpegts:....") and not contain PATCH in it ?
Created attachment 256601 [details] [review] 0001-mpegts-Add-ISO-639-parsing-functions.patch Splitting patch into three files
Created attachment 256602 [details] [review] 0002-tsdemux-Return-descriptor-in-get_descriptor.patch
Created attachment 256603 [details] [review] 0003-tsdemux-Use-mpegts-lib-for-ISO-639-language-tags.patch
commit b6d33e5ce423473a0521fe1cbe1ccf1eb78ef3f3 Author: Jesper Larsen <knorr.jesper@gmail.com> Date: Sat Oct 5 14:45:33 2013 +0200 tsdemux: Use mpegts-lib for ISO 639 language tags https://bugzilla.gnome.org/show_bug.cgi?id=709180 commit 279bdef4ea5c16ff1c0e4b291bc50f10b866e429 Author: Jesper Larsen <knorr.jesper@gmail.com> Date: Sat Oct 5 14:45:32 2013 +0200 tsdemux: Return descriptor in get_descriptor https://bugzilla.gnome.org/show_bug.cgi?id=709180 commit 7cb434e42f95590ed5e57d925cdf51e44a94d930 Author: Jesper Larsen <knorr.jesper@gmail.com> Date: Sat Oct 5 14:45:31 2013 +0200 mpegts: Add ISO 639 parsing functions https://bugzilla.gnome.org/show_bug.cgi?id=709180
This causes assertions on the file from bug #709768 : gst-launch-1.0 filesrc location=/home/slomo/Downloads/raw_h264.ts ! tsdemux ! decodebin ! xvimagesink Setting pipeline to PAUSED ... Pipeline is PREROLLING ... (gst-launch-1.0:29742): GStreamer-WARNING **: Trying to set string on taglist field 'language-code', but string is not valid UTF-8. Please file a bug. ** (gst-launch-1.0:29742): CRITICAL **: gst_mpegts_descriptor_parse_iso_639_language_idx: assertion 'descriptor->length / 4 > idx' failed (gst-launch-1.0:29742): GStreamer-WARNING **: Trying to set string on taglist field 'language-code', but string is not valid UTF-8. Please file a bug. ** (gst-launch-1.0:29742): CRITICAL **: gst_mpegts_descriptor_parse_iso_639_language_idx: assertion 'descriptor->length / 4 > idx' failed
Created attachment 256948 [details] [review] Fix assertion faults Patch that fixes the assertion faults pointed out by Sebastian. Tested with the file provided in bug #709768
commit 0d577565127433174e218545680dd3ba7cb43354 Author: Jesper Larsen <knorr.jesper@gmail.com> Date: Thu Oct 10 22:46:48 2013 +0200 mpegts: Fix assertion fault in ISO 639 parsing https://bugzilla.gnome.org/show_bug.cgi?id=709180