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 692272 - codecparsers: vc1: drop superfluous DQBEDGE
codecparsers: vc1: drop superfluous DQBEDGE
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-01-22 10:10 UTC by Gwenole Beauchesne
Modified: 2013-01-25 05:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
codecparsers: vc1: simplify GstVC1VopDquant structure (2.28 KB, patch)
2013-01-22 10:49 UTC, Gwenole Beauchesne
committed Details | Review

Description Gwenole Beauchesne 2013-01-22 10:10:01 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.
Comment 1 Tim-Philipp Müller 2013-01-22 10:31:31 UTC
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.
Comment 2 Gwenole Beauchesne 2013-01-22 10:34:36 UTC
(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.
Comment 3 Gwenole Beauchesne 2013-01-22 10:49:44 UTC
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 4 Tim-Philipp Müller 2013-01-22 13:02:02 UTC
Comment on attachment 234090 [details] [review]
codecparsers: vc1: simplify GstVC1VopDquant structure

Go for it. Thibault thinks it makes sense too.
Comment 5 Gwenole Beauchesne 2013-01-22 13:28:59 UTC
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.
Comment 6 Tim-Philipp Müller 2013-01-24 23:10:31 UTC
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.
Comment 7 Gwenole Beauchesne 2013-01-25 05:46:14 UTC
(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.