videotestsrc: reduce cpu usage for static and cachable patterns

Submitted by Laurent Glayal

Assigned to Edward Hervey @bilboed

Link to original bug (#500152)

Description

Patch to avoid computing of each new video buffer when using a unicolor one. Just keep the result in memory and copy it for each new one.

Laurent Glayal submitted a patch:

Patch 99763, "reduce CPU usage for unicolor buffers":
videotestsrc.patch

David Schleef Schleef @ds said:

The image rendering is done for every frame on purpose, so that if a downstream element stomps on the buffer (which it isn't supposed to, but...) it doesn't affect future buffers.

Also, this patch does not appear to handle changing properties correctly. It also will have problems after renegotiating caps.

Sebastian Dröge @slomo said:

Closing this as WONTFIX as the behaviour is correct.

Stefan Kost @ensonic said:

Not sure if WONTFIX is the right resolution.
Having a static-red, static-black et.c variants would be nice (does not need to be all patterns). This comes handy in testing/profiling elements.

Laurent, would you be willing to rework the patch to add the static ones as new patterns (and fix it so that it handles renegotiation)? If so, please reopen.

Stefan Kost @ensonic said:

this check is too weak:
if (v->unicolor_buffer == NULL || (v->unicolor_buffer_size != ((guint) (p->endptr - p->yp)))) {

if the yuv format changes, the size might still be the same.

Edward Hervey @bilboed said:

Sorry, I'm reopening this. The reason stated by ds in Comment 2 is unacceptable.

If some elements write in read-only buffers... those elements should be fixed.

That being said, the patch isn't good either.

Tim Müller @tpm said:

I too think the current behaviour is desirable as default, but wouldn't object if a property was added that allows buffer re-use and avoids excessive repainting.

It's not videotestsrc's job to create output that might expose bugs in elements, its job is to create as perfect a signal/stream as possible, and this includes outputting buffers with freshly allocated data for each buffer IMHO.

David Schleef Schleef @ds said:

FWIW, I don't necessarily think the current behavior is correct, I was just giving the reason why it was chosen.

The original purpose of videotestsrc was was to create the default pattern in as many formats as possible, and do it correctly and reliably. It helped uncover lots of bugs in other elements, in X drivers, etc. These days, everything else is just as reliable, plus we have a convenience library for video formats. (Which, btw, videotestsrc does not use, but that's another matter.) And now vts has lots of patterns and is useful for other purposes, so imo videotestsrc has outgrown that original purpose.

So I'm in favor of fixing up vts for repurposing. In particular, I like the idea of not regenerating the same buffer. I also like the idea of generating a scan line and then memcpy'ing a bunch of times for patterns like smpte and checkers-*. I'm also in favor of using libgstvideo. And any changes that can make it more Orc friendly.

Edward Hervey @bilboed said:

Reopening since it seems there's interest in it. Will clean-up attach my avoid-regenerating patch here soon.

Edward Hervey @bilboed submitted a patch:

For all the formats that support it and whenever downstream hasn't
changed, we returned a metadata-writable read-only copy of the
original buffer.

Patch 166393, "videotestsrc: Don't re-render every frame when it's not needed.":
videotestsrc-Dont-re-render-every-frame-when-its-n.patch

Sebastian Dröge @slomo said:

Can we commit this now?

David Schleef Schleef @ds said:

The zone plate patterns are not static, nor is ball. Setting horizontal-speed makes most patterns non-static.

I would prefer pattern_is_static to cachable_format, and make the default FALSE.

Otherwise, push it.

Julien Isorce @cap said:

I think it still make sense to have this. Especially on embedded to have a low cpu source for testing the rest of the pipeline.

Sebastian Dröge @slomo said:

Yes, just needs someone to update the patch and actually do it

Sebastian Dröge @slomo closed a related bug:

*** Bug 754632 has been marked as a duplicate of this bug. ***

Edward Hervey @bilboed said:

Assign to myself, need to refactor it.

mentioned in issue #552

mentioned in merge request !160 (merged)

mentioned in issue #576

mentioned in issue #609 (closed)