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 754291 - videoaggregator: Compositor fails with "reason not-negotiated" when changing the pixel-aspect-ratio during runtime
videoaggregator: Compositor fails with "reason not-negotiated" when changing ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 754495
 
 
Reported: 2015-08-29 16:01 UTC by Peter Körner
Modified: 2015-09-04 08:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
example program (1.71 KB, text/plain)
2015-08-29 16:01 UTC, Peter Körner
  Details
logfile of the compositor failing (level 4) (47.40 KB, text/plain)
2015-08-31 08:58 UTC, Peter Körner
  Details
videoaggregator: removing restriction on caps format changes (1.49 KB, patch)
2015-09-02 15:30 UTC, Thiago Sousa Santos
needs-work Details | Review
videoaggregator: lift restriction of changing format and pixel-aspect-ratio (1.32 KB, patch)
2015-09-02 22:40 UTC, Thiago Sousa Santos
committed Details | Review

Description Peter Körner 2015-08-29 16:01:18 UTC
Created attachment 310267 [details]
example program

The test-program attached to this bug contains a pipeline like this:

`
videotestsrc pattern=red !
video/x-raw,width=1920,height=1080,format=I420,framerate=25/1,pixel-aspect-ratio=1/1 !
videoscale !
capsfilter name=caps0 caps=video/x-raw !
mix.

compositor name=mix !
video/x-raw,width=1920,height=1080,format=I420,framerate=25/1,pixel-aspect-ratio=1/1 !
xvimagesink
`

The caps property of the capsfilter caps0 is, 1 second after the pipeline starts, set to `video/x-raw,width=950,height=534`

Because the requested resolution 950:534 is not exactly 16:9 as the input, the videoscaler will produce videoframes with a PAR of 1424/1425

`
video/x-raw, format=(string)I420, width=(int)950, height=(int)534, framerate=(fraction)25/1, pixel-aspect-ratio=(fraction)1424/1425, interlace-mode=(string)progressive
`
The compositor doesn't allow this change and terminates the pipeline.

Using videomixer instead of compositor does not show this behavior and forcing a pixel-aspect-ratio of 1/1 between the videoscaler and the compositor also fixes the issue.

What worries me is that this behavior was not visible in 1.5.1
Comment 1 Peter Körner 2015-08-31 08:58:02 UTC
Created attachment 310334 [details]
logfile of the compositor failing (level 4)
Comment 2 Thiago Sousa Santos 2015-09-02 00:03:34 UTC
Does compositor support renegotiation? It seems since 1.5 that videoaggregator (the base class) denies negotiation if the format of pixel-aspect-ratio are different.
Comment 3 Sebastian Dröge (slomo) 2015-09-02 07:10:39 UTC
It should still allow different format and par. compositor and glvideomixer both can convert between all possible raw formats.
Comment 4 Thiago Sousa Santos 2015-09-02 10:46:04 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> It should still allow different format and par. compositor and glvideomixer
> both can convert between all possible raw formats.

Yes, but is this a blocker?

@Peter: Does it work just by reverting gst-plugins-bad to 1.5.1? Or do you need to revert all modules?
Comment 5 Sebastian Dröge (slomo) 2015-09-02 11:04:33 UTC
(In reply to Thiago Sousa Santos from comment #4)
> (In reply to Sebastian Dröge (slomo) from comment #3)
> > It should still allow different format and par. compositor and glvideomixer
> > both can convert between all possible raw formats.
> 
> Yes, but is this a blocker?

The bug report says that it worked in 1.5.1, so I would consider it a blocker :)
Comment 6 Thiago Sousa Santos 2015-09-02 11:31:06 UTC
Reverting -bad to 1.5.1 works, will try git bisect.
Comment 7 Thiago Sousa Santos 2015-09-02 15:22:32 UTC
commit c28d90de68cd07a54c9bc21a5f1fde619dc81c55
Author: Thibault Saunier <tsaunier@gnome.org>
Date:   Mon Jul 6 18:51:07 2015 +0200

    videoaggregator: Remove pixel-aspect-ratio field from the caps returned by getcaps
    
    Avoiding not negotiated error while negotiating as we anyway force 1/1
    as output

Bisected to this one, the difference from 1.5.1 to master is that the negotiation in 1.5.1 would be to p-a-r=1/1 and in master is to 1424/1425, which is rejected*


* but actually tries to go and asserts ever since 
commit 6efc106a67ce6ba2a8313150ef919c0be9234da6
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Fri Mar 6 19:50:08 2015 -0500

    aggregator: Queue "latency" buffers at each sink pad.
Comment 8 Thiago Sousa Santos 2015-09-02 15:30:03 UTC
Created attachment 310515 [details] [review]
videoaggregator: removing restriction on caps format changes

videoaggregator was not allowing format, PAR or interlace mode
changes in caps, this patch lifts this restriction
Comment 9 Sebastian Dröge (slomo) 2015-09-02 15:48:49 UTC
Comment on attachment 310515 [details] [review]
videoaggregator: removing restriction on caps format changes

interlace-mode changes are still not allowed. It can't interlace or deinterlace videos, so would composite interlaced and progressive videos into the same output.

Otherwise this makes sense
Comment 10 Thiago Sousa Santos 2015-09-02 16:43:23 UTC
It seems to be lacking some proper restriction on that. The getcaps seems to report correctly the current interlace-mode but you could have 2 sink pads having its inputs being set to interlaced and non-interlaced before the source pad gets configured, leading to some inconsistency there.

Do we want to fix that here or let's postpone that to post-1.6 and just do the simple fix with a variation of the proposed patch?
Comment 11 Sebastian Dröge (slomo) 2015-09-02 17:39:55 UTC
Is the interlacing part easy/non-intrusive to fix? If not, keep it as is and just fix the PAR problem :)
Comment 12 Thiago Sousa Santos 2015-09-02 22:40:31 UTC
Created attachment 310555 [details] [review]
videoaggregator: lift restriction of changing format and pixel-aspect-ratio

The videoaggregator can convert format and PAR, there is no apparent
reason for restricting it.
Comment 13 Thiago Sousa Santos 2015-09-02 22:41:15 UTC
Seems to require some locking and no one complained, I'd defer it to after 1.6 unless someone else wants to fix it now.
Comment 14 Thiago Sousa Santos 2015-09-03 20:47:39 UTC
commit 8c5b2da124ebe5cf68eaca758dd40c92ab6228b7
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Wed Sep 2 19:16:56 2015 -0300

    videoaggregator: lift restriction of changing pixel-aspect-ratio
    
    The videoaggregator can convert PAR, there is no reason for
    restricting it.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754291