GNOME Bugzilla – Bug 754864
qtdemux: check multi trex to find track id in mp4 (DASH) stream.
Last modified: 2015-10-02 14:39:58 UTC
Created attachment 311109 [details] This is the mpd file. Overview: In case the stream has more than one trex box which is not matched to actual track id, it makes qtdemux crashed. This patch improves to check multi trex to find track id in mp4 mpeg-dash stream. Steps to Reproduce: 1) Play the attached mpd file. Actual Results: Playback crashes. Expected Results: Expectation is to play without any crash. Build Date & Hardware: Build 2014-12-18 on Ubuntu 12.04
Created attachment 311110 [details] [review] This is the diff patch.
Review of attachment 311110 [details] [review]: Thanks for the patch! ::: gst/isomp4/qtdemux.c @@ +80,2 @@ #ifdef HAVE_ZLIB +#include <zlib.h> Please don't include irrelevant changes in the same patch @@ +613,3 @@ qtdemux->flowcombiner = gst_flow_combiner_new (); + memset (qtdemux->track_fragment_id, 0, + sizeof (guint32) * GST_QTDEMUX_MAX_STREAMS); memset (qtdemux->track_fragment_id, 0, sizeof (qtdemux->track_fragment_id)); That way this code doesn't have to be changed if the array is ever changed @@ +4350,3 @@ + if (G_UNLIKELY (QTSEGMENT_IS_EMPTY (&stream-> + segments[stream->segment_index]))) { No indentation changes in the same patch please :) @@ +5102,3 @@ /* If we're doing a keyframe-only trickmode, only push keyframes on video streams */ + if (G_UNLIKELY (qtdemux->segment. + flags & GST_SEGMENT_FLAG_TRICKMODE_KEY_UNITS)) { No indentation changes of unrelated code in the same patch please :) @@ +8455,3 @@ goto skip_track; } + if (qtdemux_check_track_fragment (qtdemux, track_id)) { qtdemux_check_track_is_in_trex() or similar seems more descriptive here If I'm not mistaken you can also just use qtdemux_parse_trex() here instead, which is called many lines below in this function... instead of adding and calling the new get_track_fragment_id() @@ +8460,3 @@ + } else { + GST_WARNING_OBJECT (qtdemux, "skip track id %u, it's not fragmented",track_id); + return TRUE; There's a "skip_track" goto label for this here @@ +11645,3 @@ } + memset (qtdemux->track_fragment_id, 0,sizeof (guint32) * GST_QTDEMUX_MAX_STREAMS); + qtdemux_get_track_fragment_id (qtdemux, mvex); You don't necessarily have mvex here, see the "if (mvex)" a few lines above. It should probably be moved in that if block
So I guess qtdemux_parse_trex() should be called earlier, before the code where it currently would crash.
In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 311110 [details] [review] [review]: > > Thanks for the patch! > > ::: gst/isomp4/qtdemux.c > @@ +80,2 @@ > #ifdef HAVE_ZLIB > +#include <zlib.h> > > Please don't include irrelevant changes in the same patch On gst-indent command some changes have occurred by mistake. Will correct the same. > > @@ +613,3 @@ > qtdemux->flowcombiner = gst_flow_combiner_new (); > + memset (qtdemux->track_fragment_id, 0, > + sizeof (guint32) * GST_QTDEMUX_MAX_STREAMS); > > memset (qtdemux->track_fragment_id, 0, sizeof (qtdemux->track_fragment_id)); > > That way this code doesn't have to be changed if the array is ever changed Will incorporate the mentioned change. > > @@ +4350,3 @@ > > + if (G_UNLIKELY (QTSEGMENT_IS_EMPTY (&stream-> > + segments[stream->segment_index]))) { > > No indentation changes in the same patch please :) On gst-indent command some changes have occurred by mistake.:( > @@ +5102,3 @@ > /* If we're doing a keyframe-only trickmode, only push keyframes on video > streams */ > + if (G_UNLIKELY (qtdemux->segment. > + flags & GST_SEGMENT_FLAG_TRICKMODE_KEY_UNITS)) { > > No indentation changes of unrelated code in the same patch please :) > On gst-indent command some changes have occurred by mistake. > @@ +8455,3 @@ > goto skip_track; > } > + if (qtdemux_check_track_fragment (qtdemux, track_id)) { > > qtdemux_check_track_is_in_trex() or similar seems more descriptive here > > If I'm not mistaken you can also just use qtdemux_parse_trex() here instead, > which is called many lines below in this function... instead of adding and > calling the new get_track_fragment_id() > Will check the suggestion. > @@ +8460,3 @@ > + } else { > + GST_WARNING_OBJECT (qtdemux, "skip track id %u, it's not > fragmented",track_id); > + return TRUE; > > There's a "skip_track" goto label for this here Yes, will incorporate. > @@ +11645,3 @@ > } > + memset (qtdemux->track_fragment_id, 0,sizeof (guint32) * > GST_QTDEMUX_MAX_STREAMS); > + qtdemux_get_track_fragment_id (qtdemux, mvex); > > You don't necessarily have mvex here, see the "if (mvex)" a few lines above. > It should probably be moved in that if block I think this might not be possible, as we require for complete box parsing from mvex this function needs the mvex node qtdemux_tree_get_child_by_type_full.
GNU indent might behave slightly different with different versions. If you're working on 12.04, you could try installing a newer GNU indent ('indent' package) from a more recent ubuntu/debian version. I think it just depends on libc.
Thanks Tim :)
Created attachment 311253 [details] [review] Patch with necessary changes.
I have tried to reduce the code as u said by using qtdemux_parse_trex function instead of instead of adding and calling the new get_track_fragment_id().
Created attachment 311850 [details] [review] This patch is according to changes.
Review of attachment 311850 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +8450,3 @@ } + /* need defaults for fragments */ + qtdemux_parse_trex (qtdemux, stream, &dummy, &dummy, &dummy); Should this maybe be one line below? After the flushing? @@ -10041,3 @@ GSTTIME_TO_QTSTREAMTIME (stream, qtdemux->segment.duration); - /* need defaults for fragments */ - qtdemux_parse_trex (qtdemux, stream, &dummy, &dummy, &dummy); You should remove the dummy declaration from this block. It's unused now.
Created attachment 311919 [details] [review] Patch with changes
Comment on attachment 311919 [details] [review] Patch with changes Looks good now, thanks!
The test stream doesn't work anymore it seems, can you provide a new one? And did you actually test the latest patch to see if it really works? It seems that all your patches explicitly added the trex parsing *before* the flushing...
1. Multiple trex supporting test stream that I have is the above enclosed one. 2. Initially I had added trex parsing *before* flushing as we were checking track was fragmented based on track id. 3.Now we are using existing parse_trex function, after your comments I checked the other way of adding parse_trex *after* flushing, The scenario worked fine for me.
Thanks, makes sense :) Can you make a test stream available again nonetheless?
Created attachment 311934 [details] This is the mpd file Location :http://yahoo7p-a.akamaihd.net/2376984110001/201405/583/2376984110001_3583343755001_nonpremium-premium-nocaptions-7two-0525-lmta-011-01-4500-19ntcg6.mpd
That streams uses DRM, so doesn't really work here. Do you have a DRM-less stream? :)
Sorry :( I don't have DRM-less stream. Can you just try to remove the DRM from the stream? if possible !!! This location contains mpd of such kind, but streams are not accessible... http://bitlivedemo-a.akamaihd.net/mpds/stream.php?streamkey=bitcodin I am trying ,if possible I will give mpd of such kind.
The bitcodin one works fine here without your patches, except for bug #755115. I can't remove the DRM, sorry :) That's the whole point of DRM, no? Also I don't have any GStreamer elements that could handle this protection scheme used there.
commit e6a4c81af5fb8049d27c4bb114f3d629c2675276 Author: Manasa Athreya <manasa.athreya@lge.com> Date: Wed Sep 23 12:39:35 2015 +0900 qtdemux: Check multi trex to find track id in mp4 mpeg-dash stream If stream has more than one trex box which is not matched to actual track id, it makes qtdemux crashed. Author : Manasa Athreya (manasa.athreya@lge.com) https://bugzilla.gnome.org/show_bug.cgi?id=754864