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 782221 - jpeg2000parse does not play nice with rtpj2kpay
jpeg2000parse does not play nice with rtpj2kpay
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.12.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-05-05 13:53 UTC by Aaron Boxer
Modified: 2017-05-15 16:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix negotiation (4.97 KB, patch)
2017-05-09 09:34 UTC, Vincent Penquerc'h
none Details | Review
fix negotiation (4.99 KB, patch)
2017-05-12 08:49 UTC, Vincent Penquerc'h
committed Details | Review

Description Aaron Boxer 2017-05-05 13:53:23 UTC
The following pipeline fails negotiation:

gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! rtpj2kpay ! rtpj2kdepay ! openjpegdec ! videoconvert ! ximagesink

with the following output:

Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...
/GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0.GstPad:src: caps = video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)ARGB64, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive
/GstPipeline:pipeline0/GstOpenJPEGEnc:openjpegenc0.GstPad:src: caps = image/x-j2c, colorspace=(string)sRGB, sampling=(string)RGBA, num-components=(int)4, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)30/1, interlace-mode=(string)progressive, colorimetry=(string)sRGB, multiview-mode=(string)mono, multiview-flags=(GstVideoMultiviewFlagsSet)0:ffffffff:/right-view-first/left-flipped/left-flopped/right-flipped/right-flopped/half-aspect/mixed-mono
/GstPipeline:pipeline0/GstJPEG2000Parse:jpeg2000parse0.GstPad:sink: caps = image/x-j2c, colorspace=(string)sRGB, sampling=(string)RGBA, num-components=(int)4, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)30/1, interlace-mode=(string)progressive, colorimetry=(string)sRGB, multiview-mode=(string)mono, multiview-flags=(GstVideoMultiviewFlagsSet)0:ffffffff:/right-view-first/left-flipped/left-flopped/right-flipped/right-flopped/half-aspect/mixed-mono


while these pipelines work fine:


gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink


gst-launch-1.0 -v videotestsrc pattern=ball ! openjpegenc ! rtpj2kpay ! rtpj2kdepay ! jpeg2000parse ! openjpegdec ! videoconvert ! ximagesink
Comment 1 Aaron Boxer 2017-05-05 15:09:10 UTC
Puzzling:

rtpj2kpay only accepts "image/x-jpc" as a sink,
but negotiation never considers  "image/x-jpc"
as a possible match. 

Yet, openjpegenc can output "image/x-jpc" and jpeg2000parse
can parse "image/x-jpc".
Comment 2 Aaron Boxer 2017-05-05 19:01:15 UTC
Also, if I delete the "image/x-j2c" caps from openjpegenc source pad, and rebuild
bad plugins, then negotiation succeeds and pipeline runs. 

So, the fact that "image/x-j2c" caps exists in openjpegenc makes gstreamer attempt
to negotiate with these caps, then fails because rtpj2kpay doesn't support
"image/x-j2c", but for some reason it doesn't then try 
"image/x-jpc" which should succeed.  

Is this a bug in the negotiation code ?
Comment 3 Vincent Penquerc'h 2017-05-08 10:55:56 UTC
After some hunting, it looks like it's jpeg2000parse which can't turn one format into the other. Apparently jpc and j2c are similar formats, but jpeg2000parse will setup the source pad caps from the same media type as the sink pad caps, thus preventing any conversion from j2c to jpc. Since those are really similar, conversion should be doable.
Comment 4 Tim-Philipp Müller 2017-05-08 11:09:44 UTC
Also see bug #767546.
Comment 5 Aaron Boxer 2017-05-08 11:16:19 UTC
(In reply to Vincent Penquerc'h from comment #3)
> After some hunting, it looks like it's jpeg2000parse which can't turn one
> format into the other. Apparently jpc and j2c are similar formats, but
> jpeg2000parse will setup the source pad caps from the same media type as the
> sink pad caps, thus preventing any conversion from j2c to jpc. Since those
> are really similar, conversion should be doable.

Thanks, Vincent. Since jpeg2000parse supports both j2c and jpc, shouldn't negotiation try both formats ? Why does jpeg2000parse need to convert between the two ?
Comment 6 Vincent Penquerc'h 2017-05-08 11:17:26 UTC
Actually, even then, I think it should work, since the acceptcaps machinery should be able to work this out before then. Not quite sure how yet though.
Comment 7 Aaron Boxer 2017-05-08 11:17:50 UTC
(In reply to Tim-Philipp Müller from comment #4)
> Also see bug #767546.

Thanks. I will be working on this. But, is conversion necessary in this case ?
Comment 8 Vincent Penquerc'h 2017-05-08 11:19:27 UTC
AFAIK, negotiation will negotiate one format, but it should still be able to deal with this with the acceptcaps code, since I expect this will take x-j2c coming in, then this should be rejected by downstream. So there seems to be something buggy or missing in that part.
Comment 9 Vincent Penquerc'h 2017-05-08 11:20:16 UTC
No, I think my first comment was wrong (though conversion would also fix it, and would also be a nice to have).
Comment 10 Aaron Boxer 2017-05-08 11:38:34 UTC
(In reply to Vincent Penquerc'h from comment #8)
> AFAIK, negotiation will negotiate one format, but it should still be able to
> deal with this with the acceptcaps code, since I expect this will take x-j2c
> coming in, then this should be rejected by downstream. So there seems to be
> something buggy or missing in that part.

Yes, that's what I was thinking. I tried running with debug level of 5, but
could not pick out the problem.
Comment 11 Vincent Penquerc'h 2017-05-08 13:36:08 UTC
I'm looking at it now.
Comment 12 Aaron Boxer 2017-05-08 15:10:12 UTC
Great, thanks for your help.
Comment 13 Vincent Penquerc'h 2017-05-09 09:34:54 UTC
Created attachment 351414 [details] [review]
fix negotiation

This fixes the failing pipeline.
Comment 14 Aaron Boxer 2017-05-09 13:51:36 UTC
Review of attachment 351414 [details] [review]:

Cool. Looks good!  Thanks.
Comment 15 Aaron Boxer 2017-05-11 13:30:05 UTC
I can verify that the patch fixes the issue. What do we need to do to get this merged into master ?
Comment 16 Vincent Penquerc'h 2017-05-11 14:03:02 UTC
An OK from slomo or tpm.
Comment 17 Aaron Boxer 2017-05-11 18:35:05 UTC
alrighty, thanks.
Comment 18 Sebastian Dröge (slomo) 2017-05-11 20:22:09 UTC
Review of attachment 351414 [details] [review]:

Generally good but:

::: gst/videoparsers/gstjpeg2000parse.c
@@ +200,3 @@
+  g_return_if_fail ((in_caps == NULL) || gst_caps_is_fixed (in_caps));
+
+  caps = gst_pad_get_allowed_caps (GST_BASE_PARSE_SRC_PAD (parse));

These are not unreffed in all code paths (if they are empty)
Comment 19 Vincent Penquerc'h 2017-05-12 08:49:20 UTC
Created attachment 351703 [details] [review]
fix negotiation
Comment 20 Vincent Penquerc'h 2017-05-12 10:24:13 UTC
commit ba22007e2fa36b080754660a209668e72c910917
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Tue May 9 10:32:05 2017 +0100

    jpeg2000parse: fix negotiation with j2c and jpc both allowed upstream
    
    If upstream supports both, but downstream supports only jpc, j2c
    would have been selected as the first in the caps.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=782221



I also pushed a couple patches to fix that same leak in h264parse and h265parse, from which I had pilfered the negotiation code.
Comment 21 Aaron Boxer 2017-05-12 12:15:46 UTC
Thanks, guys!