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 754119 - gdp: Fails payloading of custom event types
gdp: Fails payloading of custom event types
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 792711 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-08-26 13:16 UTC by Sebastian Dröge (slomo)
Modified: 2018-11-03 13:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdp: increase payload type field from 16 bit to 32 bit (6.77 KB, patch)
2015-09-05 04:54 UTC, Hyunjun Ko
none Details | Review
gdp: Add new version 1.8 gdp protocol (17.32 KB, patch)
2015-09-10 09:29 UTC, Hyunjun Ko
none Details | Review
Targeted patch that moves bit 16 of GstEventType into (unused) bit 7. (1.78 KB, patch)
2018-01-20 11:11 UTC, Matt Gruenke
none Details | Review

Description Sebastian Dröge (slomo) 2015-08-26 13:16:16 UTC
GDP uses a 16 bit integer to store payload type, payload type for events is the event type + 64. All custom event types have event types > 69120.
Comment 1 Hyunjun Ko 2015-09-04 04:11:19 UTC
IMHO, there are two possible ways to fix.
1. Increase the payload type length from 16 bit to 32 bit.
2. Define new GstDPPayloadType, which presents that this is a customer event.
   And store real custom event type to 52th byte (next to dts) as 32 bit in the header.
Comment 2 Sebastian Dröge (slomo) 2015-09-04 08:33:03 UTC
I think we should go with 1), and at the same time refactor/rewrite the whole GDP thing a bit to support all the 1.x features properly. Like GstMeta...
Comment 3 Hyunjun Ko 2015-09-05 04:54:21 UTC
Created attachment 310685 [details] [review]
gdp: increase payload type field from 16 bit to 32 bit
Comment 4 Hyunjun Ko 2015-09-05 04:55:12 UTC
(In reply to Sebastian Dröge (slomo) from comment #2)
> I think we should go with 1), and at the same time refactor/rewrite the
> whole GDP thing a bit to support all the 1.x features properly. Like
> GstMeta...
Sebastian, I agree it needs to be refactored. I'm trying to provide a separated patch for this :)
Comment 5 Sebastian Dröge (slomo) 2015-09-06 15:21:38 UTC
Ideally we should provide backwards compatibility by increasing the GDP version. Also I'd like to have all things solved at once that require protocol changes, so that we don't have to break it multiple times :)
Comment 6 Hyunjun Ko 2015-09-07 07:10:58 UTC
Then.. it would be a big change :-)

Currently, gdp version is 0.2 or 1.0. 
Can I add version 1.6?
Comment 7 Sebastian Dröge (slomo) 2015-09-07 07:25:54 UTC
Seems more like this should be 1.8, but yes :)
Comment 8 Hyunjun Ko 2015-09-10 09:29:02 UTC
Created attachment 311045 [details] [review]
gdp: Add new version 1.8 gdp protocol

1. Increase payload type field from 16 bit to 32 bit to support custom event type
2. Add Meta length field for each buffer type
   - to support to serialize non-memory GstMeta in each buffer.

Sebastian.
(1)
gst_dp_payload_event_1_8
gst_dp_payload_caps_1_8
gst_dp_payload_buffer_1_8 
- This doesn't looks nice. Do you think this should be changed to something without _1_8?

(2)
+ #define GST_DP_HEADER_META_LENGTH_1_8(x)    GST_READ_UINT16_BE (x + 54)
What about this? Using this field, depay can deserialize gst meta in the buffer.
Not yet to implement to serialze/deserialize meta.

(3)
gdppay have had version property, but it was removed.
I think this property should be revived to support all versions.
Comment 9 Tim-Philipp Müller 2018-01-20 10:13:00 UTC
*** Bug 792711 has been marked as a duplicate of this bug. ***
Comment 10 Matt Gruenke 2018-01-20 11:11:10 UTC
Created attachment 367145 [details] [review]
Targeted patch that moves bit 16 of GstEventType into (unused) bit 7.

I wish this had been fixed in 1.8.  I'm not sure why it hasn't, but here's a limited patch that would fix it until a proper fix can go in.
Comment 11 GStreamer system administrator 2018-11-03 13:39:45 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-bad/issues/294.