GNOME Bugzilla – Bug 736138
validate: Use information from the media_info file to test more deeply the execution of pipeline
Last modified: 2014-11-08 14:15:16 UTC
In some cases we might want to more deeply test the execution of the pipeline on media files where we actually have information contained in the media info files. In those file we can already have metadas about all encoded frame, we might want to add the a hash for each frame so that we can check that the frames that come into the decoder (including after a seek) are the exact wanted ones. We should also be able to verify that the frames the decoder gets and outputs during backward playback are correct.
Created attachment 287596 [details] [review] validate: Add a way to pass a MediaDescriptor around monitors And add an option in gst-validate so that the user can define what media descriptor file to use.
Created attachment 287597 [details] [review] validate: Add a way to pass a MediaDescriptor around monitors And add an option in gst-validate so that the user can define what media descriptor file to use.
Created attachment 287598 [details] [review] validate: Move some method between GstMediaDescriptorParser and GstMediaDescriptor So that method land where they actually belong.
Created attachment 287599 [details] [review] validate: MediaDescriptors: Add md5sum to buffer informations In the media descriptor files, we now have the md5sum of the actual content of encoded buffers so that we can check that the buffer content is perfectly what is was supposed to be. + Fix the check of whether a frame is a keyframe in the string comparison (g_ascii_strcasecmp return 0 if string matches)
Created attachment 287600 [details] [review] validate: Add the possibility to generate media infos with frame descs + Fix a little issue when the generation fails.
Created attachment 287601 [details] [review] validate: Add more files to gitignore
Created attachment 287602 [details] [review] validate: launcher: Fix printing of errors in final report
Created attachment 287603 [details] [review] report: g_critical are CRITICAL issues!
Created attachment 287604 [details] [review] validate:media-descriptor-parser: Add a way to create from a string So it is simple to make use of it from the testsuite
Created attachment 287605 [details] [review] validate: Check all buffers when we have the info from MediaDescriptor We now check that each buffer is the expected one for each buffer that come into the decoder. + Fix some minor leaks in test-utils
Review of attachment 287597 [details] [review]: OK ::: validate/gst/validate/gst-validate-element-monitor.c @@ +83,3 @@ + break; + case GST_ITERATOR_RESYNC: + /* TODO how to handle this? */ like that ?
Review of attachment 287598 [details] [review]: ::: validate/gst/validate/media-descriptor-parser.h @@ -63,2 @@ GstClockTime gst_media_descriptor_parser_get_duration (GstMediaDescriptorParser *parser); gboolean gst_media_descriptor_parser_get_seekable (GstMediaDescriptorParser * parser); The prototypes for get_duration and get_seekable should not stay there then
Created attachment 287609 [details] [review] validate: Move some method between GstMediaDescriptorParser (Remove unimplemented prototypes as asked)
Review of attachment 287597 [details] [review]: ::: validate/gst/validate/gst-validate-element-monitor.c @@ +84,3 @@ + case GST_ITERATOR_RESYNC: + /* TODO how to handle this? */ + gst_iterator_resync (iterator); Good question xD
Review of attachment 287609 [details] [review]: OK
Review of attachment 287599 [details] [review]: OK ::: validate/gst/validate/media-descriptor-parser.c @@ +175,3 @@ GST_BUFFER_DTS (expected) = fnode->dts; if (fnode->is_keyframe) + GST_BUFFER_FLAG_UNSET (expected, GST_BUFFER_FLAG_DELTA_UNIT); The method this is contained into is confusing, and it's not in any code path (add_frame isn't used anywhere afaict)
Review of attachment 287600 [details] [review]: OK otherwise ::: validate/tools/launcher/main.py @@ +222,3 @@ + action="store_true", default=False, + help="Set it in order to generate the missing .media_infos files" + "It implies --generate-media-info but enabling frame detection") This help message could be reworded: "Same as --generate-media-info, with frame info generation" ?
Review of attachment 287601 [details] [review]: OK, my git status thanks you
Review of attachment 287602 [details] [review]: Could you explain the issue in the commit message ?
Review of attachment 287603 [details] [review]: OK
Review of attachment 287604 [details] [review]: Otherwise OK ::: validate/gst/validate/media-descriptor-parser.c @@ +406,3 @@ + parser = g_object_new (GST_TYPE_MEDIA_DESCRIPTOR_PARSER, "validate-runner", + runner, NULL); + if (_set_content (parser, g_strdup (xml), strlen (xml) * sizeof (gchar), Don't we need to free the strduped xml at some point ?
Review of attachment 287605 [details] [review]: That's a very nice feature, and an even nicer test \o/ ::: validate/gst/validate/gst-validate-pad-monitor.c @@ +1295,3 @@ + pad_monitor->check_buffers = FALSE; + } else if (!pad_monitor->caps_is_video) { + GST_DEBUG_OBJECT (pad, "Not working with video => no buffer checking"); *Only* working with video right ? @@ +1591,3 @@ + + if (pad_monitor->current_buf == NULL) { + GST_INFO_OBJECT (pad, "No current buffer one pad, Why?"); on* ::: validate/gst/validate/gst-validate-pad-monitor.h @@ +114,3 @@ + + /* GstMediaCheck related fields */ + GList *all_bufs; Why not a GSequence ? You could iterate inside it as well. ::: validate/gst/validate/gst-validate-report.h @@ +78,3 @@ #define GST_VALIDATE_ISSUE_ID_WRONG_FLOW_RETURN (((GstValidateIssueId) GST_VALIDATE_AREA_BUFFER) << GST_VALIDATE_ISSUE_ID_SHIFT | 5) #define GST_VALIDATE_ISSUE_ID_BUFFER_AFTER_EOS (((GstValidateIssueId) GST_VALIDATE_AREA_BUFFER) << GST_VALIDATE_ISSUE_ID_SHIFT | 6) +#define GST_VALIDATE_ISSUE_ID_WRONG_BUFFER (((GstValidateIssueId) GST_VALIDATE_AREA_BUFFER) << GST_VALIDATE_ISSUE_ID_SHIFT | 7) So sexy :P ::: validate/tests/check/validate/padmonitor.c @@ +382,3 @@ +/* *INDENT-OFF* */ +static const gchar * media_info = +"<file duration='10031000000' frame-detection='1' uri='file:///I/am/so/fake.fakery' seekable='true'>" seems legit
Review of attachment 287598 [details] [review]: Not OK
Review of attachment 287596 [details] [review]: Not OK
Review of attachment 287599 [details] [review]: ::: validate/gst/validate/media-descriptor-parser.c @@ +175,3 @@ GST_BUFFER_DTS (expected) = fnode->dts; if (fnode->is_keyframe) + GST_BUFFER_FLAG_UNSET (expected, GST_BUFFER_FLAG_DELTA_UNIT); Indeed, I will remove that whole method in another commit.
Created attachment 287642 [details] [review] FIX help message in "validate: Add the possibility to generate media infos with frame descs" + Fix a little issue when the generation fails. (Remove unimplemented prototypes as asked)
Created attachment 287643 [details] [review] validate: launcher: Fix printing of errors in final report We used to report issues like: ["First buffer running time was not 0",,,,,,,,,,,,,,,,] instead of: ["First buffer running time was not 0"]
Review of attachment 287604 [details] [review]: ::: validate/gst/validate/media-descriptor-parser.c @@ +406,3 @@ + parser = g_object_new (GST_TYPE_MEDIA_DESCRIPTOR_PARSER, "validate-runner", + runner, NULL); + if (_set_content (parser, g_strdup (xml), strlen (xml) * sizeof (gchar), I thought it was given to the parser, but actually we used to set it to priv->xmlcontent. Fixing that.
Created attachment 287645 [details] [review] Remove the MediaDescriptorParser->priv->xmlcontent field, and free the XML content ASAP in "validate:media-descriptor-parser: Add a way to create from a string" (Remove unimplemented prototypes as asked)
Review of attachment 287605 [details] [review]: ::: validate/gst/validate/gst-validate-pad-monitor.c @@ +1295,3 @@ + pad_monitor->check_buffers = FALSE; + } else if (!pad_monitor->caps_is_video) { + GST_DEBUG_OBJECT (pad, "Not working with video => no buffer checking"); Indeed. @@ +1591,3 @@ + + if (pad_monitor->current_buf == NULL) { + GST_INFO_OBJECT (pad, "No current buffer one pad, Why?"); FIXED ::: validate/gst/validate/gst-validate-pad-monitor.h @@ +114,3 @@ + + /* GstMediaCheck related fields */ + GList *all_bufs; I do not think it is critical here, the only moment where we will need to iterate the list is when we get a new segment. It could be an optimization in the future if we figure that it is needed. ::: validate/tests/check/validate/padmonitor.c @@ +382,3 @@ +/* *INDENT-OFF* */ +static const gchar * media_info = +"<file duration='10031000000' frame-detection='1' uri='file:///I/am/so/fake.fakery' seekable='true'>" Fakely legit, for sure!
Created attachment 287646 [details] [review] s/one/on/ in 'validate: Check all buffers when we have the info from MediaDescriptor' (Remove unimplemented prototypes as asked)
Review of attachment 287642 [details] [review]: OK
Review of attachment 287643 [details] [review]: OK thanks
Review of attachment 287645 [details] [review]: OK
Review of attachment 287646 [details] [review]: OK
Review of attachment 287597 [details] [review]: commited
Review of attachment 287599 [details] [review]: commited
Review of attachment 287601 [details] [review]: commited
Review of attachment 287609 [details] [review]: commited
Review of attachment 287603 [details] [review]: commited
Review of attachment 287642 [details] [review]: commited
Review of attachment 287643 [details] [review]: commited
Review of attachment 287645 [details] [review]: commited
Review of attachment 287646 [details] [review]: commited
Thanks for the work, closing :)