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 626518 - [imagefreeze] better caps negotiation
[imagefreeze] better caps negotiation
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal normal
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-08-10 10:38 UTC by Brandon Lewis
Modified: 2010-09-04 13:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GES debug log (ges uses fast pad linking) (725.65 KB, application/x-gzip)
2010-08-10 10:38 UTC, Brandon Lewis
  Details
python script which exemplifies the bug (1.36 KB, text/x-python)
2010-08-10 10:39 UTC, Brandon Lewis
  Details
debug log of pythong script failing on a jpeg file (552.71 KB, application/x-gzip)
2010-08-10 10:42 UTC, Brandon Lewis
  Details
debug log of python script working fine on .png file (570.54 KB, application/x-gzip)
2010-08-10 10:43 UTC, Brandon Lewis
  Details
imagefreeze: Passthrough buffer allocations (2.57 KB, patch)
2010-08-10 18:21 UTC, Sebastian Dröge (slomo)
none Details | Review
xvimagesink: It's not a bad thing if the preferred video format needs less bytes per frame (1.14 KB, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review
videoscale: Only set the PAR if the caps already had a PAR (9.00 KB, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review
videoscale: Add some debug output to the videoscale negotiation test (956 bytes, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review
xvimagesink: Suggest caps with different width/height if bufferalloc is called with impossible width/height (6.69 KB, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review
imagefreeze: Passthrough buffer allocations (2.82 KB, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review
imagefreeze: Return GST_FLOW_UNEXPECTED when getting a second buffer (1.35 KB, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review
imagefreeze: Retry bufferalloc if it was aborted with WRONG_STATE because of a flushing seek (2.29 KB, patch)
2010-08-11 16:11 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Brandon Lewis 2010-08-10 10:38:03 UTC
Created attachment 167490 [details]
GES debug log (ges uses fast pad linking)

It seems that imagefreeze sometimes fails to negotiate downstream caps properly without an ffmpegcolorspace after the freeze

this pipeline works for me:

gst-launch-0.10 uridecodebin uri=<jpeg file> ! videoscale ! ffmpegcolorspace ! imagefreeze ! xvimagesink

the situation seems to arise when imagefreeze is used inside of a gnlsource inside a composition. I'm attaching a python script which exemplifies the problem and two debug logs.




The following is an excerpt from an irc conversation:

10:24 < slomo> is there a ffmpegcolorspace between imagefreeze and the sink?
10:25 < emdash> that I don't know
10:25 < emdash> i didn't add one
10:25 < emdash> so my guess is not
10:26 -!- muelli [~muelli@port-697.pppoe.wtnet.de] has joined #gstreamer
10:26 < slomo> emdash: that's the problem then probably
10:26 < slomo> maybe we need better negotiation in imagefreeze
10:27 < slomo> please file a bug, i'll look at it later
10:27 < emdash> yeah, i'm heading home too
10:28 < slomo> ok :)
10:29 < emdash> maybe there's a reason we don't have a colorspace before the 
                sink
10:29 -!- muellisoft [~muelli@port-11268.pppoe.wtnet.de] has quit [Ping 
          timeout: 276 seconds]
10:29 < emdash> i'm looking at the code for ges-timeline-pipeline and it 
                doesn't appear to stick anything
10:29 < slomo> emdash: it shouldn't be needed
Comment 1 Brandon Lewis 2010-08-10 10:39:19 UTC
Created attachment 167491 [details]
python script which exemplifies the bug

running the script on .png files seems to work as expected for me. running the script on .jpg files (which seem to decode to yuv) does not.
Comment 2 Brandon Lewis 2010-08-10 10:42:00 UTC
Created attachment 167492 [details]
debug log of pythong script failing on a jpeg file
Comment 3 Brandon Lewis 2010-08-10 10:43:57 UTC
Created attachment 167495 [details]
debug log of python script working fine on .png file
Comment 4 Sebastian Dröge (slomo) 2010-08-10 18:21:28 UTC
Created attachment 167539 [details] [review]
imagefreeze: Passthrough buffer allocations
Comment 5 Sebastian Dröge (slomo) 2010-08-10 18:30:44 UTC
(In reply to comment #4)
> Created an attachment (id=167539) [details] [review]
> imagefreeze: Passthrough buffer allocations

So the problem here is quite complex ;)
First of all, your example only fails if the picture is larger than what XV can handle.

The problem is, that gnlsource and gnlcomposition are adding their srcpads only after a pad block (e.g. after a buffer has arrived). But that's too late to do any renegotiation, the format is already fixed and everything... and makes the linking fail.

If there's a ffmpegcolorspace and/or videoscale after imagefreeze, the pad block will already be caused by a bufferalloc. In that case downstream still has the chance (after everything is linked) to change the format and everything will work.

So the first step was, to add bufferalloc passthrough to imagefreeze. This will only work without a ffmpegcsp/vscale after imagefreeze if the decoder uses bufferalloc though, so there should always be a second ffmpegcsp/vscale after imagefreeze. But it would work in passthrough mode if the decoder did bufferalloc. For most cases you then convert the frame once before imagefreeze and everything is fine.


The remaining problem now is, that there are many pad blocks and the pad block from gnlcomposition on the srcpad of gnlsource is causing a flushing seek. This flushing seek causes the blocked bufferalloc to return wrong-state and everything fails again.
Comment 6 Sebastian Dröge (slomo) 2010-08-10 18:36:13 UTC
And I think the flushing problem is something that has to be fixed somewhere in gnonlin. For normal pipelines this is no problem because the element that handles the seek and is driving the pipeline will *pull* from upstream (or is the source) but in the case of imagefreeze (where it gets things pushed from upstream and the seek is not forwarded) this simply doesn't work because it disrupts the buffer allocation chain.
Comment 7 Sebastian Dröge (slomo) 2010-08-11 07:03:30 UTC
Ok, I have a solution in imagefreeze. Now this only needs some changes elsewhere but it can be fixed ;)
Comment 8 Sebastian Dröge (slomo) 2010-08-11 16:11:19 UTC
Created attachment 167626 [details] [review]
xvimagesink: It's not a bad thing if the preferred video format needs less bytes per frame
Comment 9 Sebastian Dröge (slomo) 2010-08-11 16:11:23 UTC
Created attachment 167627 [details] [review]
videoscale: Only set the PAR if the caps already had a PAR

Otherwise we're producing different caps and basetransform thinks that it
can't passthrough buffer allocations, etc.

In 0.11 all video caps really should have the PAR set...
Comment 10 Sebastian Dröge (slomo) 2010-08-11 16:11:27 UTC
Created attachment 167628 [details] [review]
videoscale: Add some debug output to the videoscale negotiation test
Comment 11 Sebastian Dröge (slomo) 2010-08-11 16:11:32 UTC
Created attachment 167629 [details] [review]
xvimagesink: Suggest caps with different width/height if bufferalloc is called with impossible width/height
Comment 12 Sebastian Dröge (slomo) 2010-08-11 16:11:45 UTC
Created attachment 167630 [details] [review]
imagefreeze: Passthrough buffer allocations
Comment 13 Sebastian Dröge (slomo) 2010-08-11 16:11:50 UTC
Created attachment 167631 [details] [review]
imagefreeze: Return GST_FLOW_UNEXPECTED when getting a second buffer

This prevents upstream from pushing many useless buffers and makes
it go into EOS state.
Comment 14 Sebastian Dröge (slomo) 2010-08-11 16:11:55 UTC
Created attachment 167632 [details] [review]
imagefreeze: Retry bufferalloc if it was aborted with WRONG_STATE because of a flushing seek
Comment 15 Sebastian Dröge (slomo) 2010-08-11 16:19:07 UTC
With all these patches (base patches are pushed already) this works in most cases.

A pipeline like src ! conv ! scale ! freeze ! sink will always work now *if* the source uses bufferalloc and uses a format that is supported by ffmpegcolorspace *and* videoscale. In all other cases this will still abort with not-negotiated. If you only care about png and jpeg this will always work.


If you add converters after freeze, the conversions will take place after freeze because you can only know the sink caps after the bufferalloc has advanced to the last element inside the gnlcomposition and the sink has to suggest new caps in bufferalloc. The elements most downstream will then try to establish the conversion and the conversion will take place after imagefreeze.


The other solution here would be, to add a capsfilter with the sink's caps right before imagefreeze... but that only works of course if you can know the sink caps in advance.

So for GES there are three possible solutions now:
a) if you know the sink caps in advance add a capsfilter
b) if you only care about jpeg/png only use converters before imagefreeze
c) otherwise add converters before and after imagefreeze and live with the conversions taking place after imagefreeze most of the time...
Comment 16 Tim-Philipp Müller 2010-08-12 08:39:06 UTC
How hard would it be to write a unit test that checks some of this?
Comment 17 Edward Hervey 2010-08-13 11:07:30 UTC
Looks good to me. A unit test would be nice indeed.
Comment 18 Sebastian Dröge (slomo) 2010-08-13 15:59:27 UTC
I'll make tests before pushing this... one for the bufferalloc, one for the renegotiation and one for the UNEXPECTED
Comment 19 Sebastian Dröge (slomo) 2010-09-04 13:17:00 UTC
Pushed now