New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pbutils: adding encoding-profile bindings #96

Merged
merged 6 commits into from Oct 8, 2018

Conversation

Projects
None yet
2 participants
@thiagoss
Contributor

thiagoss commented Mar 18, 2018

No description provided.

@thiagoss

This comment has been minimized.

Contributor

thiagoss commented Mar 18, 2018

This requires a small patch to pbutils in the lib, will push it after the release freeze is over

unsafe impl Sync for EncodingContainerProfile {}
pub trait EncodingContainerProfileExt {
fn add_profile<P: IsA<EncodingProfile>>(&self, profile: &P) -> Result<(), glib::error::BoolError>;

This comment has been minimized.

@sdroege

sdroege Mar 19, 2018

Owner

This one has API to modify it, but no mutexes seem to be used. So this can't be Sync, and because it's refcounted also not Send.

I would suggest to make all these objects immutable and move the mutable parts into a builder for them, so that they can be built up in the beginning but from that point onwards are immutable

fn is_enabled(&self) -> bool;
fn set_allow_dynamic_output(&self, allow_dynamic_output: bool);

This comment has been minimized.

@sdroege

sdroege Mar 19, 2018

Owner

Same thing here

status = "generate"
[[object.function]]
name = "is_equal"
ignore = true

This comment has been minimized.

@sdroege

sdroege Mar 19, 2018

Owner

Why? Please add at least a comment :)

This comment has been minimized.

@sdroege
@sdroege

This comment has been minimized.

Owner

sdroege commented Mar 19, 2018

Thanks, generally looks good except for the mutability/thread-safety mess-up that is inherited from the C API :)

@thiagoss

This comment has been minimized.

Contributor

thiagoss commented Oct 1, 2018

Tests are failing because of a problem on refcounting of the restriction caps field. It is marked as transfer-full (and allow none, which seems to be a bug as there is a GST_IS_CAPS check in the C source).

I noticed both autogenerated codes for set_format and set_restriction handle the caps as a reference and format is transfer-none and restriction is transfer-full. Shouldn't they have different handling of input parameters? The problem is that the restriction caps reference is not being lost after it is transfered to the encoding profile and later rust will try to unref it.

let restriction = restriction.into();
let restriction = restriction.to_glib_none();
unsafe {
ffi::gst_encoding_profile_set_restriction(self.to_glib_none().0, restriction.0);

This comment has been minimized.

@sdroege

sdroege Oct 1, 2018

Owner

As you say, this is transfer full. The solution here would be to use restriction.to_glib_full(), which is giving a new reference

This comment has been minimized.

@sdroege

sdroege Oct 1, 2018

Owner

And the C code is wrong and should do

g_return_if_fail (restriction == NULL || GST_IS_CAPS (restriction));

so that it actually allows NULL caps. Maybe as a workaround we can convert None here to gst::Caps::new_any() (and fix it on the C side).

This comment has been minimized.

@sdroege

sdroege Oct 1, 2018

Owner

Basically this here

diff --git a/gstreamer-pbutils/src/encoding_profile.rs b/gstreamer-pbutils/src/encoding_profile.rs
index 1ad7d4b..d3768a1 100644
--- a/gstreamer-pbutils/src/encoding_profile.rs
+++ b/gstreamer-pbutils/src/encoding_profile.rs
@@ -2,6 +2,7 @@
 use gst;
 use glib;
 use ffi;
+use gst_ffi;
 
 use std::error;
 use std::fmt;
@@ -96,9 +97,13 @@ impl<O: IsA<EncodingProfile> + IsA<glib::object::Object>> EncodingProfileEdit fo
 
     fn set_restriction<'a, P: Into<Option<&'a gst::Caps>>>(&self, restriction: P) {
         let restriction = restriction.into();
-        let restriction = restriction.to_glib_none();
         unsafe {
-            ffi::gst_encoding_profile_set_restriction(self.to_glib_none().0, restriction.0);
+            let restriction = match restriction {
+                Some(restriction) => restriction.to_glib_full(),
+                None => gst_ffi::gst_caps_new_any(),
+            };
+
+            ffi::gst_encoding_profile_set_restriction(self.to_glib_none().0, restriction);
         }
     }
 }

Fixes the warning at least

This comment has been minimized.

@thiagoss

thiagoss Oct 2, 2018

Contributor

Changed the code for this. Thanks for the diff. Will push a patch for the C code as well.

@sdroege

This comment has been minimized.

Owner

sdroege commented Oct 1, 2018

@thiagoss you also want to sign up on the freedesktop.org gitlab, and have your mail address on github public and the same mail address as one of the addresses in gitlab :) Needed for the migration to gitlab in a bit.

Thiago Santos
encoding_profile: set_restriction is transfer-full not transfer-none
Avoid refcounting mistakes by using the right converting functions
before calling the ffi layer functions
@thiagoss

This comment has been minimized.

Contributor

thiagoss commented Oct 2, 2018

Thanks for the reminder, forgot to set my default email in github.

@sdroege

Looks almost good, just lots of small things :) Thanks!

status = "generate"
[[object.function]]
name = "is_equal"
ignore = true

This comment has been minimized.

@sdroege
Show resolved Hide resolved Gir_GstPbutils.toml Outdated
Show resolved Hide resolved Gir_GstPbutils.toml
Show resolved Hide resolved examples/src/bin/encodebin.rs Outdated
Show resolved Hide resolved examples/src/bin/encodebin.rs Outdated
}
pub struct EncodingAudioProfileBuilder<'a> {
base : EncodingProfileBuilderData<'a>

This comment has been minimized.

@sdroege

sdroege Oct 2, 2018

Owner

You might want to run rustfmt :)

self
}
pub fn build(self) -> Result<EncodingProfile, EncodingProfileBuilderError> {

This comment has been minimized.

@sdroege

sdroege Oct 2, 2018

Owner

It would be nicer if this returned a Result<EncodingVideoProfile, _>

Show resolved Hide resolved gstreamer-pbutils/src/encoding_profile.rs Outdated
Show resolved Hide resolved gstreamer-pbutils/src/encoding_profile.rs Outdated
Show resolved Hide resolved gstreamer-pbutils/src/encoding_profile.rs Outdated
Thiago Santos
More encoding_profile binding improvements
- enable is_equal function again (unsure why it was disabled)
- remove restriction-caps property, encoding-profile objects are
immutable
- remove cast need by using IsA<EncodingProfile> in parameters and
returning the correct type of encodingprofile subclass from the build()
functions. It used a internal hack for storing those IsA objects in
order to keep the API clean and ready to be used, this should be sorted
out as soon as we figure out how to store them in the buidlers.
- encodebin example: remove Result error propagation when it is caused
by programming mistakes. A panic will happen in those cases.
- run rustfmt
@sdroege

This comment has been minimized.

Owner

sdroege commented on gstreamer-pbutils/src/auto/discoverer.rs in 8b0acd2 Oct 8, 2018

Oh no, you ran with --all-features :)

@sdroege

This comment has been minimized.

Owner

sdroege commented Oct 8, 2018

Looks good to me, I'll merge it now :)

@sdroege sdroege merged commit 382138b into sdroege:master Oct 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdroege

This comment has been minimized.

Owner

sdroege commented Oct 8, 2018

And will remove the automatically inserted docs, and look at the broken function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment