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 774544 - [PLUGIN-MOVE] Move rawparse to gst-plugins-base
[PLUGIN-MOVE] Move rawparse to gst-plugins-base
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 776420
Blocks:
 
 
Reported: 2016-11-16 14:26 UTC by Sebastian Dröge (slomo)
Modified: 2017-02-25 12:58 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Sebastian Dröge (slomo) 2016-11-16 14:26:23 UTC
.
Comment 1 Sebastian Dröge (slomo) 2016-11-27 11:22:35 UTC
... or maybe to -base?

I'll go over the checklist in the next days.

Biggest question is probably if we want to also move the compatibility audioparse/videoparse bins, or if they should go away or stay in -bad.
Comment 2 Tim-Philipp Müller 2016-11-27 11:33:18 UTC
-base works for me.

I was wondering however how much value this rawbase class really adds, if we shouldn't split out the audio and the video parsers and move them into different audio/video specific plugins, e.g. could go next to audio/video parsers or next to audio/video convert (I think the scalers/resamplers should be merged with the convert plugin as well btw, so it doesn't seem too much of a stretch to add those there too ;))

Could someone list in what respect audioparse/videoparse are incompatible with the new elements?
Comment 3 Sebastian Dröge (slomo) 2016-11-27 11:53:15 UTC
(In reply to Tim-Philipp Müller from comment #2)
> -base works for me.
> 
> I was wondering however how much value this rawbase class really adds, if we
> shouldn't split out the audio and the video parsers and move them into
> different audio/video specific plugins, e.g. could go next to audio/video
> parsers or next to audio/video convert (I think the scalers/resamplers
> should be merged with the convert plugin as well btw, so it doesn't seem too
> much of a stretch to add those there too ;))

The (private!) base class reduces quite some code, just look at it ;)

> Could someone list in what respect audioparse/videoparse are incompatible
> with the new elements?

Different property names at least
Comment 4 Tim-Philipp Müller 2016-11-27 12:04:26 UTC
> The (private!) base class reduces quite some code, just look at it ;)

I was asking this after looking at it ;)
Comment 5 Sebastian Dröge (slomo) 2016-12-05 12:45:52 UTC
I'd suggest we a) move it in -base, b) as one plugin for now, and c) later consider merging various plugins that belong together into one (audioconvert/resample/volume/parse | videoconvert/scale/parse) for which we can then put the baseclass into gst-plugins-base/gst/common (or so) and just build it into both plugins (with appropriate -D for renaming).
Comment 6 Tim-Philipp Müller 2016-12-05 18:06:50 UTC
> I'd suggest we a) move it in -base, b) as one plugin for now, and c) later
> consider merging various plugins that belong together into one
> (audioconvert/resample/volume/parse | videoconvert/scale/parse)

Works for me.


> for which we can then put the baseclass into gst-plugins-base/gst/common
> (or so) and just build it into both plugins (with appropriate -D for
> renaming).

Eww. (Let's just worry about this when the time comes ;))
Comment 7 Tim-Philipp Müller 2016-12-05 18:50:20 UTC
At least two things left to do before this can happen:

1) Bug #773666 (on my list, but not top of my list)

2) need to decide what to do with audioparse/videoparse - keep them, as-is, just make subclasses with compat property names, drop them? We can probably even provide the old versions with the same code and some different property/type names via some defines, it looks like the renames were mostly gratuitious / for clarity.
Comment 8 Olivier Crête 2016-12-05 22:21:34 UTC
I'd prefer if we kept the old *parse APIs in place.. My vote would be to have a compatible subclass.
Comment 9 Sebastian Dröge (slomo) 2016-12-16 16:16:34 UTC
(In reply to Tim-Philipp Müller from comment #7)
> At least two things left to do before this can happen:
> 
> 1) Bug #773666 (on my list, but not top of my list)

Not sure if that should necessarily be a blocker, we could for now also just let it drop incomplete data instead of asserting (rawparse that is). It would have the same effect and does not involve possibly breaking other parsers.

> 2) need to decide what to do with audioparse/videoparse - keep them, as-is,
> just make subclasses with compat property names, drop them? We can probably
> even provide the old versions with the same code and some different
> property/type names via some defines, it looks like the renames were mostly
> gratuitious / for clarity.

We have subclasses with compat names already, registered with the names of the old elements. The old code is all gone.

(In reply to Olivier Crête from comment #8)
> I'd prefer if we kept the old *parse APIs in place.. My vote would be to
> have a compatible subclass.

That's exactly what is there

I would vote for just keeping the compat elements for now (in -bad!) and move everything else to -base into a new rawparse plugin.
Comment 10 Sebastian Dröge (slomo) 2017-01-23 10:44:00 UTC
Tim, any opinions? I would move forward with the code as-is plus having it handle incomplete data on draining.

Apart from that it all seems ready but I'll double check again later if you agree.
Comment 11 Tim-Philipp Müller 2017-01-23 10:51:57 UTC
I'm ok with that.
Comment 12 Sebastian Dröge (slomo) 2017-02-18 18:20:39 UTC
commit 63e280df22073f06dc35a5cab03dad5d509eca71
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Feb 18 20:18:50 2017 +0200

    rawbaseparse: Drop incomplete frames at EOS
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=773666
    
    This would ideally be solved in baseparse but that requires further
    thought at this point, and in the meantime it would be good to have
    rawbaseparse not assert on this but handle it gracefully instead.



Now I'm only waiting for a comment in bug #776420
Comment 13 Sebastian Dröge (slomo) 2017-02-25 12:58:13 UTC
Moved to -base