GNOME Bugzilla – Bug 343988
data protocol needs extending to handle events better
Last modified: 2006-06-06 16:47:43 UTC
attaching a patch that keeps ABI compatibility for the 0.2 version, and adds a GDP 1.0 version
Created attachment 66811 [details] [review] adds a packetizer struct, an enum, and some support code, to have both 0.2 and 1.0
"This data protocol assumes an underlying lossless connection, such as" - standard terminology is "a reliable connection-oriented transport". Can we call the new protocol version "0.3" or "0.10", reserving "1.0" for the version of DP corresponding to a hypothetical future gstreamer 1.0? /* we only copy KEY_UNIT,DELTA_UNIT and IN_CAPS flags */ - why only these? The only buffer flag that looks like it shouldn't be copied to me is readonly, since we always make a writable buffer on the other side. This isn't new in your patch, but is there a way to check whether debugging is on? The stuff in gst_dp_dump_byte_array() is quite expensive even when debugging is off, and should be skipped in that case. + /* FIXME: GST_IS_CAPS doesn't work + g_return_val_if_fail (GST_IS_CAPS (caps), FALSE); */ - please find out why, fix, and reinstate this check. In gst_dp_packet_from_caps_any, avoid calling strlen multiple times. Fairly trivial, but it's good practice. Using a NULL terminator here is bad protocol design, perhaps you can fix this for the new version? Why does GstDPPacketizer have major, minor members, rather than a single GstDPVersion member? Do you want ABI padding in this struct? Basic idea looks solid.
terminology changed. for major/minor versioning, what I wanted to do was to use major for incompatible changes, and minor for compatible changes/additions. Tying it to the GStreamer version in some way would not achieve much; the major number would be in effect useless. I'd prefer to have the version number reflect some state of compatibility of the protocol. Maybe I should add a note about this ? Copied everything except READONLY flags. the dump_byte_array in my opinion can go. don't know if there's a way to check the debug levels at any given point. the FIXME is for later, unrelated to what I'm adding. strlen only called once. packetizer->version changed attaching new patch
Created attachment 66813 [details] [review] update patch
Fine. Ok now; would like to see the note about versioning added, though.
ok, adding the note. commited to CVS, thanks for reviewing.