GNOME Bugzilla – Bug 729382
Add a name property to GESTimelineElement
Last modified: 2014-09-23 09:16:51 UTC
This way it is simpler to find the in a timeline. This allows us to implement easily more actionf for GstValidate. Attaching patches that also add new actions
Created attachment 275634 [details] [review] tools: Position printing is now done at the gst-validate level
Created attachment 275635 [details] [review] Add a notion of 'name' in GESTimelineElement
Created attachment 275636 [details] [review] tools:validate: Add an action to allow editing clips
Created attachment 275637 [details] [review] scenario: Add a scenario that edits a clip while the pipeline is paused
Created attachment 275638 [details] [review] ges-validate: Add a scenario to set children properties
Created attachment 275639 [details] [review] tools:validate: Always seek after editing a clip Otherwize the displayed frame will not be updated when paused.
Created attachment 275640 [details] [review] tools:validate: Add an action to scerialize the project
Created attachment 275641 [details] [review] ges: Rename remaning tlobj to clip
Created attachment 275642 [details] [review] tools: Disable --set-scenario if not compiled against gst-validate
Created attachment 275643 [details] [review] tools: Handle times as doubles + concider duration=0 as TIME_NONE
Created attachment 275644 [details] [review] tools: Always activate gst-validate to have position printing
Created attachment 275645 [details] [review] project: Enhance debugging when badly updating a URI
Created attachment 275646 [details] [review] tools: Add an option to disable mixing
Created attachment 275647 [details] [review] tools: Add a way to look for moved media sample recursively In ges-launch let the user set a folder where the media sample that move can be found recursing into that specified folder.
Review of attachment 275634 [details] [review]: OK
Review of attachment 275635 [details] [review]: OK ::: ges/ges-timeline-element.c @@ +296,3 @@ + goto had_parent; + + g_free (self->name); You could get that before the check for parent and remove the g_free in had_parent.
Review of attachment 275636 [details] [review]: Not OK, I think some of these comments need to be addressed. ::: tools/ges-validate.c @@ -44,0 +46,15 @@ +static gboolean +_edit_clip (GstValidateScenario * scenario, GstValidateAction * action) +{ ... 12 more ... Unneeded new line :) @@ -44,0 +46,17 @@ +static gboolean +_edit_clip (GstValidateScenario * scenario, GstValidateAction * action) +{ ... 14 more ... You need to either check that it was correctly obtained or make validate enforce mandatory fields I think. Even though passing NULL to get_element should also return NULL @@ +65,3 @@ + g_return_val_if_fail (timeline, FALSE); + + clip = ges_timeline_get_element (timeline, clip_name); get_element is (transfer-full) right ? @@ +81,3 @@ + gst_validate_utils_enum_from_str (GES_TYPE_EDGE, edge_str, &edge); + + gst_structure_get_int (action->structure, "new-layer-priority", If new-layer-priority is not provided we will end up editing to a prio of -1 I think
Review of attachment 275637 [details] [review]: OK
Review of attachment 275638 [details] [review]: The name of the commit is a bit off I think, it's rather about adding an action. Not OK ::: tools/ges-validate.c @@ +60,3 @@ + g_return_val_if_fail (GES_IS_TRACK_ELEMENT (element), FALSE); + + property_name = gst_structure_get_string (action->structure, "property"); Same comment, mandatory blabla @@ -123,2 +150,4 @@ move_clip_mandatory_fields, "Allows to seek into the files", FALSE); + gst_validate_add_action_type ("set-child-property", _set_child_property, + move_clip_mandatory_fields, move_clip_mandatory_fields ? shouldn't it be a new array ?
Review of attachment 275639 [details] [review]: Split that commit please :)
Review of attachment 275640 [details] [review]: OK ::: tools/ges-validate.c @@ +84,3 @@ +_serialize_project (GstValidateScenario * scenario, GstValidateAction * action) +{ + const gchar *uri = gst_structure_get_string (action->structure, "uri"); Same here, let's make sure validate enforces the mandatory fields
Review of attachment 275641 [details] [review]: OK but the commit name is playing tricks on my brain :)
Review of attachment 275642 [details] [review]: OK
Review of attachment 275643 [details] [review]: OK
Review of attachment 275644 [details] [review]: OK
Review of attachment 275645 [details] [review]: OK, maybe make the commit name a bit more explicit ?
Review of attachment 275646 [details] [review]: OK
Review of attachment 275647 [details] [review]: OK, did you try having media with the same name at different levels of the folder structure when recusring though ?
(In reply to comment #17) > Review of attachment 275636 [details] [review]: > > Not OK, I think some of these comments need to be addressed. > > ::: tools/ges-validate.c > @@ -44,0 +46,15 @@ > +static gboolean > +_edit_clip (GstValidateScenario * scenario, GstValidateAction * action) > +{ > ... 12 more ... > > Unneeded new line :) > > @@ -44,0 +46,17 @@ > +static gboolean > +_edit_clip (GstValidateScenario * scenario, GstValidateAction * action) > +{ > ... 14 more ... > > You need to either check that it was correctly obtained or make validate > enforce mandatory fields I think. Even though passing NULL to get_element > should also return NULL Mandatory should be mandatory and in action implementation it is right to concider you will find them I think. > @@ +65,3 @@ > + g_return_val_if_fail (timeline, FALSE); > + > + clip = ges_timeline_get_element (timeline, clip_name); > > get_element is (transfer-full) right ? Indeed. > @@ +81,3 @@ > + gst_validate_utils_enum_from_str (GES_TYPE_EDGE, edge_str, &edge); > + > + gst_structure_get_int (action->structure, "new-layer-priority", > > If new-layer-priority is not provided we will end up editing to a prio of -1 I > think And -1 means "No move" :)
(In reply to comment #29) > Review of attachment 275639 [details] [review]: > > Split that commit please :) Will just fixup the part on .scenario to the commit it was introduced in
Review of attachment 275636 [details] [review]: ::: tools/ges-validate.c @@ -44,0 +46,15 @@ +static gboolean +_edit_clip (GstValidateScenario * scenario, GstValidateAction * action) +{ ... 12 more ... Removed @@ +65,3 @@ + g_return_val_if_fail (timeline, FALSE); + + clip = ges_timeline_get_element (timeline, clip_name); Fixed @@ +81,3 @@ + gst_validate_utils_enum_from_str (GES_TYPE_EDGE, edge_str, &edge); + + gst_structure_get_int (action->structure, "new-layer-priority", Not an issue
Review of attachment 275638 [details] [review]: Changed the commit name to ges-validate: Add a GstValidate action to set children properties ::: tools/ges-validate.c @@ -123,2 +150,4 @@ move_clip_mandatory_fields, "Allows to seek into the files", FALSE); + gst_validate_add_action_type ("set-child-property", _set_child_property, + move_clip_mandatory_fields, Added an array like: const gchar *set_child_property_mandatory_fields[] = { "element-name", "property", "value", NULL }; They all are mandatory
Review of attachment 275639 [details] [review]: Splitted
Review of attachment 275645 [details] [review]: Changed to "project: Enhance debugging when updating URI with a bad URI"
commit 5785178e521c87d5ec58cf3351bb5e818fe55327 Author: Thibault Saunier <tsaunier@gnome.org> Date: Thu May 1 10:13:39 2014 +0200 tools: Add a way to look for moved media sample recursively In ges-launch let the user set a folder where the media sample that move can be found recursing into that specified folder. https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 2dd59ab53a485bd00b372497fc9d602235ed4317 Author: Thibault Saunier <tsaunier@gnome.org> Date: Wed Apr 30 20:58:42 2014 +0200 tools: Add an option to disable mixing + Add a a GObject property so that the info is seralized https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit cba79c09acda63d9e06b1fd076e773ed79f91961 Author: Thibault Saunier <tsaunier@gnome.org> Date: Wed Apr 30 16:26:03 2014 +0200 project: Enhance debugging when updating URI with an invalid one https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 92b115d3e0d3fdb6e448ae9e1fe5643300be6173 Author: Thibault Saunier <tsaunier@gnome.org> Date: Fri May 2 16:49:10 2014 +0200 tools: Always activate gst-validate to have position printing https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 36cc9f13ec89f2ec2ff63f29f1a7f22e893f0306 Author: Thibault Saunier <tsaunier@gnome.org> Date: Tue Apr 29 21:29:54 2014 +0200 tools: Handle times as doubles + concider duration=0 as TIME_NONE https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 44cbc27cb831575f81dc9684ebf05be647f232c2 Author: Thibault Saunier <tsaunier@gnome.org> Date: Sat Apr 26 09:51:37 2014 +0200 tools: Disable --set-scenario if not compiled against gst-validate https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit ff1446c2b28a8b896a45a3a17a669fac39975b9b Author: Thibault Saunier <tsaunier@gnome.org> Date: Sat Apr 26 08:55:31 2014 +0200 ges: Rename remaning tlobj to clip https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 8601666a5aca1ed318fc6a91e56c8e37f01c4b95 Author: Thibault Saunier <tsaunier@gnome.org> Date: Fri May 2 16:43:42 2014 +0200 ges-validate: Add an action to serialize the project https://bugzilla.gnome.org/show_bug.cgi?id=729382 Conflicts: tools/ges-validate.c commit 4ce52d8a62b18c0cd9d0bb6c5ff5eea194ccf251 Author: Thibault Saunier <tsaunier@gnome.org> Date: Fri Apr 25 18:23:06 2014 +0200 tools:validate: Always seek after editing a clip Otherwize the displayed frame will not be updated when paused. + Add a get_timeline internal helper method in ges-validate.c https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 9cb7d8e3e7892e477e0ddcacf9bef3694dcfd9f9 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Fri Mar 14 12:10:53 2014 +0100 ges-validate: Add a GstValidate action to set children properties https://bugzilla.gnome.org/show_bug.cgi?id=729382 commit 0b0bbdddd0f1c05fb9a88b38ebce9696fd346c40 Author: Thibault Saunier <thibault.saunier@collabora.com> Date: Tue Feb 18 18:52:38 2014 +0100 scenario: Add a scenario that edits a clip while the pipeline is paused https://bugzilla.gnome.org/show_bug.cgi?id=729382