[Bug 668483] video: add support for global-alpha multiplicator for overlay rectangles

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=668483
  GStreamer | gst-plugins-base | git

--- Comment #13 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2012-03-15 23:46:02 UTC ---
> - As proposed per-rectangle seq_num is updated automatically if global_alpha is
> changed.

About the seq_num: I'm a bit undecided here. On the one hand, the seq_nums are
in the end just an optimisation - if they are useful, fine, if not, tough luck.
So updating the seq_num in _rectangle_set_global_alpha() isn't wrong and
correctly signals that for all practical purposes the rectangle has changed. So
it's an ok thing to do. We can go with that.

However, from a consumer/overlay perspective, what one actually wants to know
if whether the underlying pixel data changed, right? For a consumer that
doesn't support global alpha, it doesn't make a difference. But for a consumer
who does support global alpha - they want to know that the pixels are the same
as last time, and only the global alpha changed, so they can use the old object
and transform it accordingly. Or at least that's how I imagine it. Is this what
APIs would support? Or does it not work like that anyway? (Ultimately it's just
an optimisation, so not a blocker). If so, one could introduce an additional
seqnum for the underlying pixels excl. global alpha basically. But then,
perhaps a consumer that does support global alpha should just check the
GstBuffer pointers it gets - we could put a seqnum into GST_BUFFER_OFFSET() or
so which could be used along the same lines.

On a side note, if the seqnum of the rectangle is updated like that in
_rectangle_set_global_alpha(), the composition the rectangle belongs to (if
any) won't know about the change, so min_seq_num_used might not be as high as
it could be. Having said that, we don't provide API to query min_seq_num yet,
so it doesn't matter anyway.


> - The current implementation is 'optimized' for speed and saving memory. I
> first tried an implementation integrating nicely into the internal caching of
> preprocessed/scaled rectangles, meaning that after each application of
> global_alpha the resulting pixel-data was added to the rectangle cache. This
> turned out to be too inefficient in terms of CPU-usage and memory consumption

Why inefficient in terms of CPU usage?

As for memory consumption: there's no reason why one can't remove cached
rectangles. It would be perfectly reasonable to only keep "the last" modified
rectangle around.

> and made osd-fading pratically unusable in our application on STBs. Therefore I
> kept caching only for scaled and un/premultiplied rectangle-data and
> implemented the application of global_alpha to be done on the fly in
> get_pixels_argb_internal() on the pixel-data per scaled rectangle. I think this
> is reasonable as the need for caching of faded overlays is questionable.

That's fine with me. It's an implementation detail / internal optimisation
anyway, and could still be added later if desired.

So the case where you need to apply global alpha on the rectangle, but no
scaling is needed/wanted is handled in the "scaling code path" as well, right?


Some nitpicks:

 - in gst_video_overlay_rectangle_extract_alpha() and
    gst_video_overlay_rectangle_apply_global_alpha() please
    use the ARGB_A define I added with the above commits
    (it could be derived from the format using gst_video_*
    API, but there's no particular benefit at this point while
    we only support one format. Providing the compiler with
    the fixed number might lead to marginally faster code.)

 - update c1,c2,c3, alpha_channel in apply_global_alpha()
   to new ARGB_* defines as well

 - g_free() is NULL-safe (re. finalize and extract_alpha)

 - no need to check the return value of g_malloc(), it will
   always suceed (or abort). There's g_try_malloc() if
   desired, but probably not needed here. Just assume it
   will work and do away with the non-error handling in
   apply_global_alpha() and extract_alpha() I'd say :-)

 - in apply_global_alpha(), how sure are you that no one
   else is holding a ref to rect->pixels, meaning it mustn't
   me modified? perhaps do a gst_buffer_make_writable()
   which will make a copy if anyone else is holding a ref
   and keep the original buf if not. (This then leads us
   back to the seqnum question I guess...)

 - just use 4 instead of bpp everywhere

- would really love some unit tests, but not a deal
   breaker ;)

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.