[Bug 754864] qtdemux:check multi trex to find track id in mp4 (DASH) stream.

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=754864

--- Comment #4 from manasa.athreya at lge.com ---
In reply to Sebastian Dröge (slomo) from comment #2)
> 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

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.