After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796716 - encoding: Introduce ih264enc to unify H264 video encoder APIs
encoding: Introduce ih264enc to unify H264 video encoder APIs
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: High critical
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-06-28 19:49 UTC by Thibault Saunier
Modified: 2018-11-03 12:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
encoding: Add an ih264enc to unify H264 video encoder APIs (30.38 KB, patch)
2018-06-28 19:49 UTC, Thibault Saunier
none Details | Review
openh264: Implement the GstH264EncoderInterface (2.01 KB, patch)
2018-06-28 19:50 UTC, Thibault Saunier
none Details | Review
x264enc: Implement the GstH264EncoderInterface interface (1.83 KB, patch)
2018-06-28 19:52 UTC, Thibault Saunier
none Details | Review

Description Thibault Saunier 2018-06-28 19:49:10 UTC
See commit message.
Comment 1 Thibault Saunier 2018-06-28 19:49:17 UTC
Created attachment 372868 [details] [review]
encoding: Add an ih264enc to unify H264 video encoder APIs

This introduces two different components:

- A GstH264EncoderInterface aiming at offering a common API for
  encoders that support it. It only exposes vmethod and no properties
  as the goal is to not break the current behaviour of the already
  exposed properties. For example, h264 encoders already expose a
  "bitrate" property but some expose it in bits per seconds and others
  in kbits per second, we can't change that. Also the default values
  for those properties are different but we want them to be all equal
  when harmonizing the properties for a particular codec. This interface
  is not meant to be used directly but only through the ih264enc
  element.

- A GstIH264Enc wrapper which exposes the common properties and make use
  of the new GstH264EncoderInterface in order to set them with a same
  semantic and default values.

In the end, the user should use the ih264enc wrapper and set properties
on it and expect that the behaviour of those is identical whatever
encoder is selected.

Currently only a "bitrate" property is exposed and the goal is to
harmonize APIs step by step.

A GstIVideoEnc baseclass is also introduced so that it is simpler
to add more API harmonizing elements in the future.

This is all introduced in -base as the interface needs to be implemented
in the various h264 encoders that are in many different places.
Comment 2 Thibault Saunier 2018-06-28 19:50:47 UTC
Created attachment 372869 [details] [review]
openh264: Implement the GstH264EncoderInterface
Comment 3 Thibault Saunier 2018-06-28 19:52:51 UTC
Created attachment 372870 [details] [review]
x264enc: Implement the GstH264EncoderInterface interface
Comment 4 Mathieu Duponchelle 2018-06-28 19:55:55 UTC
Review of attachment 372868 [details] [review]:

::: gst-libs/gst/video/gsth264encoderinterface.h
@@ +40,3 @@
+ * GstH264EncoderInterfaceInterface:
+ * @parent: parent interface type.
+ * @set_bitrate: Set bitrate (in bits per second)

You probably want to document the expected behaviour more specifically what should the behaviour be at the bounds, does a bitrate of 0 mean "automatic" like some encoders?
Comment 5 Thibault Saunier 2018-06-28 20:01:47 UTC
(In reply to Mathieu Duponchelle from comment #4)
> Review of attachment 372868 [details] [review] [review]:
> 
> ::: gst-libs/gst/video/gsth264encoderinterface.h
> @@ +40,3 @@
> + * GstH264EncoderInterfaceInterface:
> + * @parent: parent interface type.
> + * @set_bitrate: Set bitrate (in bits per second)
> 
> You probably want to document the expected behaviour more specifically what
> should the behaviour be at the bounds, does a bitrate of 0 mean "automatic"
> like some encoders?

Right, although the meaning and the way it is used should be exclusively handled by the wrapper (0 is not acceptable at that point). I think we will want an extra property to represent that.
Comment 6 Olivier Crête 2018-06-28 20:22:19 UTC
Instead of having this wrapper stuff. Why not make an interface that is defined in a document and then use regular properties (ie not a GInterface at all)? So for example, we'd document property names like "h264-bitrate", "h264-min-qp", "h264-keyframe-interval", "h264-pass", their type and the max range that they can have. But each element would just implement each property "normally" and for each property, each element would be able to expose the min/max that is really supports and it would just work with "normal" gst-inspect. We can enforce it with some unit test or gst-validate test.
Comment 7 Thibault Saunier 2018-06-28 20:31:00 UTC
(In reply to Olivier Crête from comment #6)
> Instead of having this wrapper stuff. Why not make an interface that is
> defined in a document and then use regular properties (ie not a GInterface
> at all)? So for example, we'd document property names like "h264-bitrate",
> "h264-min-qp", "h264-keyframe-interval", "h264-pass", their type and the max
> range that they can have. But each element would just implement each
> property "normally" and for each property, each element would be able to
> expose the min/max that is really supports and it would just work with
> "normal" gst-inspect. We can enforce it with some unit test or gst-validate
> test.

Basically the idea is to be able to uniformize step by step leading to a clean interface, adding new properties to existing elements would not achieve that fmpov.

What you describe here doesn't fully work for the same reason a GInterface without the wrapper doesn't really work , that is:

- How do you handle defaults? For example, you have vaapiench264::bitrate (kbps)  default = 0 (mean automatic), that can't be changed, so if you add h264-bitrate, what would be the default? You can't change the current default of any of the subclasses. The wrapper allow that in a nice way fmpov. 
- You end up with namespacing, making the API harder to understand

Also I believe a GInterface with properties would be cleaner than just a design doc, why do you think you don't want a GInterface?
Comment 8 Olivier Crête 2018-06-28 20:40:06 UTC
(In reply to Thibault Saunier from comment #7)
> Also I believe a GInterface with properties would be cleaner than just a
> design doc, why do you think you don't want a GInterface?

My main problem with a GInterface is that you end up having the same min/max/etc for all implementations, so you can't use that to document restrictions.

Also, Nicolas mentioned that there may be problems when you add properties to a GInterface later.. that you end up having to implement it in all users, otherwise you get nasty warnings in gst-inspect.
Comment 9 Thibault Saunier 2018-06-28 20:46:30 UTC
(In reply to Olivier Crête from comment #8)
> (In reply to Thibault Saunier from comment #7)
> > Also I believe a GInterface with properties would be cleaner than just a
> > design doc, why do you think you don't want a GInterface?
> 
> My main problem with a GInterface is that you end up having the same
> min/max/etc for all implementations, so you can't use that to document
> restrictions.

You can yes.

> Also, Nicolas mentioned that there may be problems when you add properties
> to a GInterface later.. that you end up having to implement it in all users,
> otherwise you get nasty warnings in gst-inspect.

Not sure it is exactly a bad thing in itself.

Though the mechanism proposed in this patches "solves" it all and more fmpov. The goal here is to provide one implementation agnostic interface that behaves as similarly as possible whatever the implementation is, the goal being to hide encoder particularities to the end user. What you describe here is not fulfilling those the requirements fmpov. What is your concerned about the proposed approach?
Comment 10 Olivier Crête 2018-06-29 15:04:54 UTC
(In reply to Thibault Saunier from comment #9)
> (In reply to Olivier Crête from comment #8)
> > (In reply to Thibault Saunier from comment #7)
> > > Also I believe a GInterface with properties would be cleaner than just a
> > > design doc, why do you think you don't want a GInterface?
> > 
> > My main problem with a GInterface is that you end up having the same
> > min/max/etc for all implementations, so you can't use that to document
> > restrictions.
> 
> You can yes.

How?

> Though the mechanism proposed in this patches "solves" it all and more
> fmpov. The goal here is to provide one implementation agnostic interface
> that behaves as similarly as possible whatever the implementation is, the
> goal being to hide encoder particularities to the end user. What you
> describe here is not fulfilling those the requirements fmpov. What is your
> concerned about the proposed approach?

I dislike the added complexity of the wrapper bin, I find it super ugly. And it doesn't offer a way to know which controls are supported by a specific encoder and which values are actually valid.. Ideally in a programatic way, only including properties that are implemented with the limits that match reality sounds like the easiest.
Comment 11 Thibault Saunier 2018-06-29 16:05:09 UTC
(In reply to Olivier Crête from comment #10)
> (In reply to Thibault Saunier from comment #9)
> > (In reply to Olivier Crête from comment #8)
> > > (In reply to Thibault Saunier from comment #7)
> > > > Also I believe a GInterface with properties would be cleaner than just a
> > > > design doc, why do you think you don't want a GInterface?
> > > 
> > > My main problem with a GInterface is that you end up having the same
> > > min/max/etc for all implementations, so you can't use that to document
> > > restrictions.
> > 
> > You can yes.
> 
> How?

Just install the same property on subclasses. 

> > Though the mechanism proposed in this patches "solves" it all and more
> > fmpov. The goal here is to provide one implementation agnostic interface
> > that behaves as similarly as possible whatever the implementation is, the
> > goal being to hide encoder particularities to the end user. What you
> > describe here is not fulfilling those the requirements fmpov. What is your
> > concerned about the proposed approach?
> 
> I dislike the added complexity of the wrapper bin, I find it super ugly.

Implementation wise it looks a bit weird maybe, but complex? Let me disagree.

> And it doesn't offer a way to know which controls are supported by a specific
> encoder and which values are actually valid.. Ideally in a programatic way,
> only including properties that are implemented with the limits that match
> reality sounds like the easiest.

But this is the exact goal of it! Hiding away specificities of of each implementation is what we want here, if you want to know about them, then don't use that API.
Comment 12 Olivier Crête 2018-06-29 20:40:21 UTC
(In reply to Thibault Saunier from comment #11)
> > And it doesn't offer a way to know which controls are supported by a specific
> > encoder and which values are actually valid.. Ideally in a programatic way,
> > only including properties that are implemented with the limits that match
> > reality sounds like the easiest.
> 
> But this is the exact goal of it! Hiding away specificities of of each
> implementation is what we want here, if you want to know about them, then
> don't use that API.

I don't think you can validly do this. If you specify "b-frames=0" and you get b-frames, this is really counterproductive, it's better to get an error.
Comment 13 Víctor Manuel Jáquez Leal 2018-07-02 17:16:38 UTC
Which software does require this API? encodebin? which else?
Comment 14 Thibault Saunier 2018-07-13 11:19:54 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #13)
> Which software does require this API? encodebin? which else?

It is not really related to encodebin itself, it is more for any application that use encoders giving them a way to treat all encoder for a given media type the exact same way. I am doing that for WebKit webrtc and PiTiVi, both would need something like that.
Comment 15 GStreamer system administrator 2018-11-03 12:08:23 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/466.