GNOME Bugzilla – Bug 748390
validate: media-descriptor: remove duplicate conditions
Last modified: 2015-08-16 13:37:13 UTC
when comparing tags, two conditions in if an else if are same the correct way is to first check if both are NULL and return. changed the condition accordingly.
Created attachment 302270 [details] [review] change duplicate conditions
Can someone review this :)
Review of attachment 302270 [details] [review]: You are changing the bahaviour here, we will just never get into the else if branch in that code, we might juste remove it? Don't know.
Hi Thibault, As per my understanding with the present code, it will never enter else if But my change is if (!rtags && !ctags) so it will enter if condition, when rtags == NULL and ctags == NULL first else if condition is when rtags == NULL and ctags has value second else if condition is when rtags has value and ctags == NULL. Regards, Vineeth
Hi Thibault, I just rechecked the code. The function is to compare the tags of two streams being passed as input. After my patch, when both tags are NULL it is just returning without doing any comparison. and the else if conditions are added only to print report mentioning about the presence of tag in one and not in another. The actual comparison happens only when both the tags are present which is present in the end of the function. Before my patch, when both rtags and ctags are NULL, then there is no condition to check it and exit. Hence the comparison logic will be called and this will result in crash. Regards, Vineeth
ping :)
Created attachment 307888 [details] [review] correct trivial spelling mistake To make more sense of the issue, the return values should be handled properly. First things first, corrected a spelling mistake in function name.
Created attachment 307889 [details] [review] handle proper return values This patch handles the return values being provided by compare_tags and returns accordingly.
Created attachment 307890 [details] [review] change duplicate conditions With the handling of return values in the above patch, this will make more sense. We can probably merge all of these together. But just to maintain history added as different patches.
Review of attachment 307888 [details] [review]: OK
Review of attachment 307889 [details] [review]: ::: validate/gst/validate/media-descriptor.c @@ -304,3 +304,3 @@ } - compare_tags (ref, rstream, cstream); + return compare_tags (ref, rstream, cstream); I think it was on purpose so that it does not fail for tags issue (but simply WARNS the user)
Review of attachment 307890 [details] [review]: OK
(In reply to Thibault Saunier from comment #11) > Review of attachment 307889 [details] [review] [review]: > > ::: validate/gst/validate/media-descriptor.c > @@ -304,3 +304,3 @@ > } > > - compare_tags (ref, rstream, cstream); > + return compare_tags (ref, rstream, cstream); > > I think it was on purpose so that it does not fail for tags issue (but > simply WARNS the user) Yes i just now noticed the commit in which that change was made.. should we change the return type to void.. it doesn't matter actually..
Created attachment 308052 [details] [review] handle proper return values compare_tags is not a fatal error. Hence not checking the return value.
Review of attachment 308052 [details] [review]: ok
Ok, we must wait before merging these. I simply run validate on top, and there is 25 failing tests now. We need to figure-out is these are valid but not visible failure, or if this is test regression. Vineeth, did you run the test afterward ?
Marking them as need-work, so we get good explanation of why these regression are normal.
Review of attachment 308052 [details] [review]: I really wonder why we do media check if non of the test cause failure/regression ? ::: validate/tools/gst-validate-media-check.c @@ +122,3 @@ + if (!gst_media_descriptors_compare (GST_MEDIA_DESCRIPTOR (reference), + GST_MEDIA_DESCRIPTOR (writer))) { + ret = 1; This is the only thing that can cause a test to fail now. It looks like stream ID are no longer the same as when the media info was first generated.
OK, short story, stream-ID are not guarantied to be stable across sessions. For filesrc base IDs, the ID varies depending on the path the asset is loaded from. Making adding this strict comparison will always fail for someone. In fact, we should drop stream-id from the recorded media-info.
Arguably, a stream-id change could indicate a regression, hence this check should remain. In bug 753079, I'm suggesting some solutions to reduce the amount of false positive warnings. In any case, we need to remove the code that makes it fail the media-check.
commit e25a6aaf78eb592315f4cc990f1bee0494420241 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Jul 31 10:49:00 2015 -0400 validate: media-descriptor: Fix reading seekable record Casting the result of g_strmp0 to boolean won't make gboolean value 0 or 1. We need proper 0 and 1 so we can use == comparision. commit 8d477c6d930c3a7251a7fbdac5879d643639bf76 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Fri Jul 24 15:36:27 2015 +0900 validate: media-descriptor: handle proper return values while comparing the media descriptor with --expected-results, the return values are not being handled properly, which results in wrong comparision https://bugzilla.gnome.org/show_bug.cgi?id=748390 commit bd5fb7be260dd8a38ae069c131fae48f688bb22a Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Thu Jul 30 15:14:13 2015 -0400 validate: media-descriptor: Add comment before ignored return value As stated in the bug, this comparison failing is not a critical error, warning is enough. Add a comment so nobody thinks it's a coding error. https://bugzilla.gnome.org/review?bug=748390 commit 2a5ff3f3c8dbc0edd57c3d19a2963639a80b0056 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Wed Jul 22 16:32:06 2015 +0900 validate: media-descriptor: remove duplicate conditions when comparing tags, two conditions in if an else if are same the correct way is to first check if both are NULL and return. changed the condition accordingly. https://bugzilla.gnome.org/show_bug.cgi?id=748390 commit f020c41d41358af35da09ce5127c722df1d9845e Author: Vineeth TM <vineeth.tm@samsung.com> Date: Wed Jul 22 16:07:19 2015 +0900 validate: media-descriptor: fix trivial spelling mistakes replace comparse_stream with compare_streams https://bugzilla.gnome.org/show_bug.cgi?id=748390