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 719734 - gst-rtsp-server: allow setting a different rtp manager
gst-rtsp-server: allow setting a different rtp manager
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-rtsp-server
git master
Other Linux
: Normal enhancement
: 1.2.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-03 00:13 UTC by Aleix Conchillo Flaqué
Modified: 2014-02-25 22:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add create_rtpbin vmethod (8.79 KB, patch)
2013-12-03 00:20 UTC, Aleix Conchillo Flaqué
none Details | Review
add create_rtpbin vmethod (fix) (8.80 KB, patch)
2013-12-03 18:30 UTC, Aleix Conchillo Flaqué
none Details | Review
add create_rtpbin in media not factory (3.53 KB, patch)
2013-12-03 19:58 UTC, Aleix Conchillo Flaqué
none Details | Review

Description Aleix Conchillo Flaqué 2013-12-03 00:13:52 UTC
Currently, rtsp-media uses a hardcoded rtpbin element. It would be good if rtpbin could be constructed by the user (in the media factory) and pass it to the media as with gst_rtsp_media_take_pipeline.

Right now, the rtpbin can be configured if rtsp-media is subclassed by implementing setup_rtpbin.
Comment 1 Aleix Conchillo Flaqué 2013-12-03 00:20:14 UTC
Created attachment 263356 [details] [review]
add create_rtpbin vmethod

Add a create_rtpbin vmethod in the media factory. This makes it easy to create a different rtpbin.

With this the setup_rtpbin vmethod might not make more sense as the rtpbin can now be setup when creating it.
Comment 2 Aleix Conchillo Flaqué 2013-12-03 18:30:52 UTC
Created attachment 263420 [details] [review]
add create_rtpbin vmethod (fix)

Better to keep the state_lock while the rtpbin is updated.
Comment 3 Tim-Philipp Müller 2013-12-03 18:41:10 UTC
Is this for your srtpbin? Will it still be needed once you modify rtpbin?
Comment 4 Aleix Conchillo Flaqué 2013-12-03 19:25:50 UTC
Yes, srtpbin won't exist anymore. It's for the modified rtpbin so I can connect signals to it and don't need to subclass media.

I could do it in the setup_rtpbin vmethod from rtsp-media, but I think the the create_rtpbin vmethod in the factory is more flexible.

But right now I'm thinking this is wrong, as you can prepare and unprepare the media and prepare it again.
Comment 5 Aleix Conchillo Flaqué 2013-12-03 19:28:28 UTC
I already have a modified rtpbin which adds the following signals:

  request-rtp-encoder
  request-rtp-decoder
  request-rtcp-encoder
  request-rtcp-decoder
  link-rtp-encoder
  link-rtp-decoder
  link-rtcp-encoder
  link-rtcp-decoder

It works, but I'm still playing with it. Too many signals for my taste, but couldn't find any other way to do it. Still working on it.
Comment 6 Aleix Conchillo Flaqué 2013-12-03 19:36:06 UTC
(In reply to comment #4)

> But right now I'm thinking this is wrong, as you can prepare and unprepare the
> media and prepare it again.

What about changing the vmethod setup_rtpbin in the media for create_rtpbin?

Or simply adding create_rtpbin?
Comment 7 Olivier Crête 2013-12-03 19:44:47 UTC
Why the link signals? And the RTP and RTCP encoder need to be the same element if they share the same master key.
Comment 8 Aleix Conchillo Flaqué 2013-12-03 19:58:20 UTC
Created attachment 263434 [details] [review]
add create_rtpbin in media not factory

This one solves the prepare/unprepare/prepare. It adds a create_rtpbin to media so one can change the rtp manager. It is not necessary for the srtpbin (as it won't exist) but who knows if one wants to use a different manager.

Which means I will need to subclass rtsp-media for my purposes.
Comment 9 Aleix Conchillo Flaqué 2013-12-03 20:08:15 UTC
(In reply to comment #7)
> Why the link signals? And the RTP and RTCP encoder need to be the same element
> if they share the same master key.

Yes, I guess we don't need two signal and the linking can be also done in the request signal.

About RTP and RTCP encoder, I thought it was possible to have separate master keys (in the srtpenc case it would need two elements), I think the RFC allows this. But may be I am wrong.
Comment 10 Aleix Conchillo Flaqué 2013-12-03 20:17:48 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > Why the link signals? And the RTP and RTCP encoder need to be the same element
> > if they share the same master key.
> 
> Yes, I guess we don't need two signal and the linking can be also done in the
> request signal.
> 

Oh, wait, now I remember why two signals. The first one (request) will return the element (so it can be freed later), and the second one (link) will return the pad that needs to be ghosted.
Comment 11 Aleix Conchillo Flaqué 2013-12-03 23:19:25 UTC
(In reply to comment #3)
> Is this for your srtpbin? Will it still be needed once you modify rtpbin?

I am now subclassing rtsp-media and use setup_rtpbin, but I think giving the option of creating a different rtpbin (via create_rtpbin) it's not bad thing.
Comment 12 Aleix Conchillo Flaqué 2013-12-06 20:04:16 UTC
To make sure this is the last comment and it doesn't get lost, patch in comment 8 would still be valid if you find it useful.
Comment 13 Wim Taymans 2013-12-09 16:22:04 UTC
commit ab3651d3393d3771975e6f1180ea9adf1fde0605
Author: Aleix Conchillo Flaqué <aleix@oblong.com>
Date:   Tue Dec 3 11:54:42 2013 -0800

    media: add new create_rtpbin vmethod
    
    * gst/rtsp-server/rtsp-media.[ch]: add new create_rtpbin vmethod.
    
      https://bugzilla.gnome.org/show_bug.cgi?id=719734
Comment 14 Tim-Philipp Müller 2013-12-09 16:44:11 UTC
Aleix, just in case you weren't aware - adding the new vfunc 'in the middle' of the GstRTSPMediaClass structure breaks ABI technically. If there had been a 1.x release yet, that would have been a problem.
Comment 15 Aleix Conchillo Flaqué 2013-12-09 18:46:36 UTC
(In reply to comment #14)
> Aleix, just in case you weren't aware - adding the new vfunc 'in the middle' of
> the GstRTSPMediaClass structure breaks ABI technically. If there had been a 1.x
> release yet, that would have been a problem.

Right. I knew, but I really didn't think about it when I made the patch. Thanks for the heads up!