GNOME Bugzilla – Bug 692272
codecparsers: vc1: drop superfluous DQBEDGE
Last modified: 2013-01-25 05:46:14 UTC
Hi, DQDBEDGE and DQSBEDGE can share the same variable in GstVC1VopDquant. Currently, we only use DQSBEDGE. So, one of the DQSBEDGE and DQBEDGE could be removed. I suggest we remove DQSBEDGE since DQBEDGE can express both, depending on DQPROFILE. What do you think? If a patch is accepted for this one, it would only go to git master due to the API/ABI change.
I'm actually a bit confused by the comments in the header file, do they make sense at all? What you suggest makes sense as far as I can tell: rename dqsbedge to dqbedge, and remove dqbedge, right? We could just rename dqbedge to 'unused' or so with a 'FIXME: remove' comment, then we break API but maintain ABI compatibility.
(In reply to comment #1) > I'm actually a bit confused by the comments in the header file, do they make > sense at all? I think this is what Thibault meant: let dqbedge represent DQSBEDGE if dqprofile == GST_VC1_DQPROFILE_SINGLE_EDGE or DQDBEDGE if dqprofile == GST_VC1_DQPROFILE_DOUBLE_EDGE instead. > What you suggest makes sense as far as I can tell: rename dqsbedge to dqbedge, > and remove dqbedge, right? Yes, this is exactly what I had in mind. > We could just rename dqbedge to 'unused' or so with a 'FIXME: remove' comment, > then we break API but maintain ABI compatibility. Sure. I will cook a patch with this idea.
Created attachment 234090 [details] [review] codecparsers: vc1: simplify GstVC1VopDquant structure Here is the suggested change. Rename dqsbedge to dqbedge. The intent is that we can only have a single boundary edge selector, depending on the value of dqprofile. So, dqbedge represents DQSBEDGE if dqprofile == GST_VC1_DQPROFILE_SINGLE_EDGE, or DQDBEDGE if dqprofile == GST_VC1_DQPROFILE_DOUBLE_EDGE. The former dqbedge field is marked as unused and can be removed on the next gst-plugins-bad version that allows ABI changes.
Comment on attachment 234090 [details] [review] codecparsers: vc1: simplify GstVC1VopDquant structure Go for it. Thibault thinks it makes sense too.
Pushed to git master only. Thanks. Can I propagate this to older 0.10 and 1.0 branches? This is not really important since we only used dqsbedge anyway. So, I don't mind.
I would prefer if you didn't push it to the other branches, esp. not 0.10, since it might break compilation with older code. It would probably be okay to push to 1.0 as well though, since I believe you're the only user in this case, but it'd be better if you could work around it in gst-vaapi IMHO.
(In reply to comment #6) > I would prefer if you didn't push it to the other branches, esp. not 0.10, > since it might break compilation with older code. > > It would probably be okay to push to 1.0 as well though, since I believe you're > the only user in this case, but it'd be better if you could work around it in > gst-vaapi IMHO. OK, I will keep this one to master. For gst-vaapi, I always use codecparsers from git master anyway. I maintain a mirror here: http://gitorious.org/vaapi/gstreamer-codecparsers/ Rationale: people don't always want to install from git, and since I don't want to be bugged for the same issues ever again, I simply make sure to use the latest codecparsers (built-in as a submodule). There is a branch called "gst-vaapi-rebased" where the only differences should be either patches I forgot to submit, or patches that didn't went in yet.