GNOME Bugzilla – Bug 750863
tests: dashdemux: added unit tests for parsing period element
Last modified: 2015-06-25 08:13:29 UTC
Created attachment 305155 [details] [review] path to extend dash_mpd unit tests Improved dash_mpd unit tests by adding new tests that parse the Period element. Code coverage reported by lcov for dash/gstmpdparser.c is: lines......: 43.0% (984 of 2287 lines) functions..: 47.5% (67 of 141 functions)
Comment on attachment 305155 [details] [review] path to extend dash_mpd unit tests Please split this patch a bit. There are at least three independent changes: changing times from gint64 to guint64 (maybe put that into the other bug you created about that?), the addition of the new tests you mention here, and some additional checks in existing tests.
Created attachment 305321 [details] [review] split the original patch into 5 parts. This is 1/5 As requested, I split the original patch into 5 parts. This is 1/5
Created attachment 305322 [details] [review] split the original patch into 5 parts. This is 2/5
Created attachment 305323 [details] [review] split the original patch into 5 parts. This is 3/5
Created attachment 305324 [details] [review] split the original patch into 5 parts. This is 4/5
Created attachment 305325 [details] [review] split the original patch into 5 parts. This is 5/5
Review of attachment 305321 [details] [review]: ::: tests/check/elements/dash_mpd.c @@ -121,3 @@ assert_equals_int (gst_date_time_get_second (availabilityEndTime), 50); assert_equals_int64 (mpdclient->mpd_node->mediaPresentationDuration, Why not assert_equals_uint64() instead?
Review of attachment 305323 [details] [review]: ::: tests/check/elements/dash_mpd.c @@ +59,3 @@ GstMpdClient *mpdclient = gst_mpd_client_new (); + ret = gst_mpd_parse (mpdclient, xml, (gint) strlen (xml)); Does it make sense to use gint for this function? Shouldn't it be gsize or guint instead?
Review of attachment 305325 [details] [review]: Looks generally good, but needs changing if the first two patches need changes ::: tests/check/elements/dash_mpd.c @@ +354,3 @@ + GstMpdClient *mpdclient = gst_mpd_client_new (); + + ret = gst_mpd_parse (mpdclient, xml, (gint) strlen (xml)); Same comment about the gint here @@ +360,3 @@ + assert_equals_string (periodNode->id, "TestId"); + assert_equals_int64 (periodNode->start, + (gint64) duration_to_ms (0, 1, 2, 12, 10, 20, 123)); and assert_equals_uint64() here
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 305321 [details] [review] [review]: > > ::: tests/check/elements/dash_mpd.c > @@ -121,3 @@ > assert_equals_int (gst_date_time_get_second (availabilityEndTime), 50); > > assert_equals_int64 (mpdclient->mpd_node->mediaPresentationDuration, > > Why not assert_equals_uint64() instead? Because mediaPresentationDuration has type gint64. I also think it should be unsigned, so I raised a ticket to review all durations and change their type from signed to unsigned (https://bugzilla.gnome.org/show_bug.cgi?id=750847).
(In reply to Sebastian Dröge (slomo) from comment #8) > Review of attachment 305323 [details] [review] [review]: > > ::: tests/check/elements/dash_mpd.c > @@ +59,3 @@ > GstMpdClient *mpdclient = gst_mpd_client_new (); > > + ret = gst_mpd_parse (mpdclient, xml, (gint) strlen (xml)); > > Does it make sense to use gint for this function? Shouldn't it be gsize or > guint instead? The declaration of gst_mpd_parse is: gboolean gst_mpd_parse (GstMpdClient *client, const gchar *data, gint size); In my opinion size should have the type gsize. But I don't want to make changes in the interface of dashdemux parser. I don't think somebody will use a negative value for size, but you never know.
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 305325 [details] [review] [review]: > > Looks generally good, but needs changing if the first two patches need > changes > > ::: tests/check/elements/dash_mpd.c > @@ +354,3 @@ > + GstMpdClient *mpdclient = gst_mpd_client_new (); > + > + ret = gst_mpd_parse (mpdclient, xml, (gint) strlen (xml)); > > Same comment about the gint here > > @@ +360,3 @@ > + assert_equals_string (periodNode->id, "TestId"); > + assert_equals_int64 (periodNode->start, > + (gint64) duration_to_ms (0, 1, 2, 12, 10, 20, 123)); > > and assert_equals_uint64() here start has type gint64. I don't think it can be negative, but until we correct its type, we must use assert_equals_int64
(In reply to Florin Apostol from comment #11) > (In reply to Sebastian Dröge (slomo) from comment #8) > > Review of attachment 305323 [details] [review] [review] [review]: > > > > ::: tests/check/elements/dash_mpd.c > > @@ +59,3 @@ > > GstMpdClient *mpdclient = gst_mpd_client_new (); > > > > + ret = gst_mpd_parse (mpdclient, xml, (gint) strlen (xml)); > > > > Does it make sense to use gint for this function? Shouldn't it be gsize or > > guint instead? > > The declaration of gst_mpd_parse is: > gboolean gst_mpd_parse (GstMpdClient *client, const gchar *data, gint size); > > In my opinion size should have the type gsize. But I don't want to make > changes in the interface of dashdemux parser. I don't think somebody will > use a negative value for size, but you never know. Change it in a separate patch then, thanks :)
(In reply to Florin Apostol from comment #10) > (In reply to Sebastian Dröge (slomo) from comment #7) > > Review of attachment 305321 [details] [review] [review] [review]: > > > > ::: tests/check/elements/dash_mpd.c > > @@ -121,3 @@ > > assert_equals_int (gst_date_time_get_second (availabilityEndTime), 50); > > > > assert_equals_int64 (mpdclient->mpd_node->mediaPresentationDuration, > > > > Why not assert_equals_uint64() instead? > > Because mediaPresentationDuration has type gint64. I also think it should be > unsigned, so I raised a ticket to review all durations and change their type > from signed to unsigned (https://bugzilla.gnome.org/show_bug.cgi?id=750847). Alright, let's handle that in the other bug then :)
Thank you for applying the patches. You forgot to review and apply patch 2/5