Created attachment 266475 [details] [review]
Patch to fixe the above issues

Review of attachment 266475 [details] [review]:

::: /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
@@ +235,3 @@
+  if(!dfbpool->dfbvideosink->ext_surface) {
+	  ret = dfbpool->dfbvideosink->dfb->CreateSurface (dfbpool->dfbvideosink->dfb,
+													   &s_dsc, &surface);

Please run gst-indent on this and create a new patch :)

@@ +284,2 @@
   /* Release our internal surface */
+  if (meta && meta->surface) {

Why would we ever have no meta? Sounds like we should just fail in that case

@@ +435,3 @@
      this is an optimisation for memory allocation */
+  if (meta->pixel_format != DSPF_UNKNOWN) {
+	  alloc_size = meta->width * meta->height * DFB_BYTES_PER_PIXEL(meta->pixel_format);

What about stride here?

@@ +1130,3 @@
+	  case GST_VIDEO_FORMAT_BGR:
+	  case GST_VIDEO_FORMAT_RGB:
+	  pixel_format = DSPF_RGB24;

Can you put this part in a separate patch? Also I don't think it's correct. It's probably GST_VIDEO_FORMAT_BGR on little endian, and RGB on big endian.

(In reply to comment #2)
> Review of attachment 266475 [details] [review]:
> 
> :::
> /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
> @@ +235,3 @@
> +  if(!dfbpool->dfbvideosink->ext_surface) {
> +      ret = dfbpool->dfbvideosink->dfb->CreateSurface
> (dfbpool->dfbvideosink->dfb,
> +                                                       &s_dsc, &surface);
> 
> Please run gst-indent on this and create a new patch :)
> 
> @@ +284,2 @@
>    /* Release our internal surface */
> +  if (meta && meta->surface) {
> 
> Why would we ever have no meta? Sounds like we should just fail in that case
> 

I think that in the case where we use ext_surface, the code just allocate a standard GstBuffer (not 100% sure) have to look deeper at the code 

> @@ +435,3 @@
>       this is an optimisation for memory allocation */
> +  if (meta->pixel_format != DSPF_UNKNOWN) {
> +      alloc_size = meta->width * meta->height *
> DFB_BYTES_PER_PIXEL(meta->pixel_format);
> 
> What about stride here?
To be honest, I don't know what is the stride ^^
> 
> @@ +1130,3 @@
> +      case GST_VIDEO_FORMAT_BGR:
> +      case GST_VIDEO_FORMAT_RGB:
> +      pixel_format = DSPF_RGB24;
> 
> Can you put this part in a separate patch? Also I don't think it's correct.
> It's probably GST_VIDEO_FORMAT_BGR on little endian, and RGB on big endian.

Yes, this is a quit fix for my case of use to work, but this certainly needs some tests of this find :)

Any advice on how to split a patch file?

Created attachment 266477 [details] [review]
Fixed issues related to ext_surface usage.

Created attachment 266478 [details] [review]
Fixes RGB BGR issue

need to be enhanced with bif/little endian flags

After checking the code, it seems to me it's normal to have meta == NULL cause in _pool_alloc_buffer, in the fallback: section, we have surface = gst_buffer_new_allocate. then in beach: *buffer = surface;

So it seems to me the meta is lost here...

Review of attachment 266478 [details] [review]:

::: /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
@@ +1125,1 @@
     case GST_VIDEO_FORMAT_RGB:

As said, this looks wrong and the same in the other change. Use things like 
#if G_BYTE_ORDER == G_LITTLE_ENDIAN
blablabla code with BGR
#else
blablabla code with RGB
#endif

Assuming this is really correct with DirectFB. I didn't check that.

Review of attachment 266477 [details] [review]:

Please also provide both patches in "git format-patch" format.

::: /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
@@ +436,3 @@
+  if (meta->pixel_format != DSPF_UNKNOWN) {
+    alloc_size =
+        meta->width * meta->height * DFB_BYTES_PER_PIXEL (meta->pixel_format);

With stride I mean how many bytes are used per row, which is usually not width*bpp but larger than that

(In reply to comment #9)
> Review of attachment 266477 [details] [review]:
> 
> Please also provide both patches in "git format-patch" format.
> 
> :::
> /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
> @@ +436,3 @@
> +  if (meta->pixel_format != DSPF_UNKNOWN) {
> +    alloc_size =
> +        meta->width * meta->height * DFB_BYTES_PER_PIXEL (meta->pixel_format);
> 
> With stride I mean how many bytes are used per row, which is usually not
> width*bpp but larger than that

OK so its the same has pitch?

So alloc_size would be height * stride?

(In reply to comment #10)

> > With stride I mean how many bytes are used per row, which is usually not
> > width*bpp but larger than that
> 
> OK so its the same has pitch?
> 
> So alloc_size would be height * stride?

Yes, pitch is also used as name for that... however sometimes people express pitch in pixels, which is dangerous and often useless ;) Check what DirectFB does here... if it's in bytes then the allocation size will be height*stride for all RGB formats.

(In reply to comment #11)
> (In reply to comment #10)
> 
> > > With stride I mean how many bytes are used per row, which is usually not
> > > width*bpp but larger than that
> > 
> > OK so its the same has pitch?
> > 
> > So alloc_size would be height * stride?
> 
> Yes, pitch is also used as name for that... however sometimes people express
> pitch in pixels, which is dangerous and often useless ;) Check what DirectFB
> does here... if it's in bytes then the allocation size will be height*stride
> for all RGB formats.

I think this should do : 
    alloc_size = meta->height * DFB_BYTES_PER_LINE(meta->pixel_format, meta->width);

Yes, that looks good

Created attachment 266534 [details] [review]
Fixes issues related to ext_surface usage.

tried using git for the diff, hope this is right

Created attachment 266535 [details] [review]
Fixes RGB BGR issue

Not the best patch, but solved my case :)

(In reply to comment #9)
> Review of attachment 266477 [details] [review]:
> 
> Please also provide both patches in "git format-patch" format.
> 
> :::
> /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
> @@ +436,3 @@
> +  if (meta->pixel_format != DSPF_UNKNOWN) {
> +    alloc_size =
> +        meta->width * meta->height * DFB_BYTES_PER_PIXEL (meta->pixel_format);
> 
> With stride I mean how many bytes are used per row, which is usually not
> width*bpp but larger than that

In DirectFB they use the term pitch, see Lock method of Surface, ret_pitch: http://directfb.org/docs/DirectFB_Reference_1_7/IDirectFBSurface_Lock.html

(In reply to comment #16)
> (In reply to comment #9)
> > Review of attachment 266477 [details] [review] [details]:
> > 
> > Please also provide both patches in "git format-patch" format.
> > 
> > :::
> > /home/etrousset/source_ext/gst-1.2.2/gst-plugins-bad-1.2.2.modified/ext/directfb/dfbvideosink.c
> > @@ +436,3 @@
> > +  if (meta->pixel_format != DSPF_UNKNOWN) {
> > +    alloc_size =
> > +        meta->width * meta->height * DFB_BYTES_PER_PIXEL (meta->pixel_format);
> > 
> > With stride I mean how many bytes are used per row, which is usually not
> > width*bpp but larger than that
> 
> In DirectFB they use the term pitch, see Lock method of Surface, ret_pitch:
> http://directfb.org/docs/DirectFB_Reference_1_7/IDirectFBSurface_Lock.html

yes, but with DFB_BYTES_PER_LINE(meta->pixel_format,
meta->width); we can avoid Lock and Unlocking the surface just to get the pitch.

(In reply to comment #17)
> yes, but with DFB_BYTES_PER_LINE(meta->pixel_format,
> meta->width); we can avoid Lock and Unlocking the surface just to get the
> pitch.

DFB_BYTES_PER_LINE() is the minimum stride allowed for the format. It fine if you need to allocate a surface from system memory, or for backend implementation, but on DirectFB application side I don't see why you would ever want to do that. I'll need to have a bigger look at the context.

(In reply to comment #18)
> (In reply to comment #17)
> > yes, but with DFB_BYTES_PER_LINE(meta->pixel_format,
> > meta->width); we can avoid Lock and Unlocking the surface just to get the
> > pitch.
> 
> DFB_BYTES_PER_LINE() is the minimum stride allowed for the format. It fine if
> you need to allocate a surface from system memory, or for backend
> implementation, but on DirectFB application side I don't see why you would ever
> want to do that. I'll need to have a bigger look at the context.

I think you are right, it would be better to use the actual pitch of the surface, in stead of guessing... I'll change the patch accordingly

Created attachment 266558 [details] [review]
Update to ext_surface patch

Comment on attachment 266535 [details] [review]
Fixes RGB BGR issue

It's probably the same problem for xRGB and ARGB. Or there is no problem at all :)

Review of attachment 266558 [details] [review]:

::: ext/directfb/dfbvideosink.c
@@ +440,3 @@
+    meta->dfbvideosink->ext_surface->Lock(meta->dfbvideosink->ext_surface, DSLF_READ, &data, &pitch);
+    alloc_size = meta->height * pitch;
+    meta->dfbvideosink->ext_surface->Unlock(meta->dfbvideosink->ext_surface);

This is likely to be completely wrong for multi-planar formats like I420. See the code a few lines above that handles those and does basically the same.

Also all this all (including existing code) can only work properly if we can allocate with videometa... which upstream explicitly has to set on the buffer pool.

(In reply to comment #21)
> (From update of attachment 266535 [details] [review])
> It's probably the same problem for xRGB and ARGB. Or there is no problem at all
> :)

I'll have to try this...

(In reply to comment #22)
> Review of attachment 266558 [details] [review]:
> 
> ::: ext/directfb/dfbvideosink.c
> @@ +440,3 @@
> +    meta->dfbvideosink->ext_surface->Lock(meta->dfbvideosink->ext_surface,
> DSLF_READ, &data, &pitch);
> +    alloc_size = meta->height * pitch;
> +    meta->dfbvideosink->ext_surface->Unlock(meta->dfbvideosink->ext_surface);
> 
> This is likely to be completely wrong for multi-planar formats like I420. See
> the code a few lines above that handles those and does basically the same.
> 
> Also all this all (including existing code) can only work properly if we can
> allocate with videometa... which upstream explicitly has to set on the buffer
> pool.

I think I almost have a patch that leads to less code duplication/code path differences. I'll post it as soon as I fix a blinking issue :)

I find the source of my blinking, it looks like at first, two buffers are allocated. Initially I had blink because the two buffers where linked to the same DFB surface (the initial back buffer). So I flipped my Surface so that each buffer allocation is linked to the respective surface buffer

But when I change the scaling of the video, the two buffer are released, and only 1 is created. the video is blinking again cause only 1 surface buffer is written! Any idea why?

You probably have to keep an additional reference of the last rendered buffer in the sink, and only release it (and give it back to the pool and let it be used upstream) after the next buffer is rendered.

(In reply to comment #26)
> You probably have to keep an additional reference of the last rendered buffer
> in the sink, and only release it (and give it back to the pool and let it be
> used upstream) after the next buffer is rendered.

I'm afraid I don't get it!
Do you mean I should keep pointer to the rendered buffer and unref the n-1 one?

Actually, with my latest modification, I do not allocate memory any more, I use the one from the ext_surface, just as it was one allocated by the sink.

But I think I need to handle the case of a DOUBLE buffer surface.

(In reply to comment #27)
> (In reply to comment #26)
> > You probably have to keep an additional reference of the last rendered buffer
> > in the sink, and only release it (and give it back to the pool and let it be
> > used upstream) after the next buffer is rendered.
> 
> I'm afraid I don't get it!
> Do you mean I should keep pointer to the rendered buffer and unref the n-1 one?

I meant that your render function looks like this:

render(sink, buf) {
  if (sink->last_buffer)
    gst_buffer_unref(sink->last_buffer);
  sink->last_buffer = gst_buffer_ref(buf);
  do_dfb_stuff_to_render(buf);
}

(In reply to comment #29)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > You probably have to keep an additional reference of the last rendered buffer
> > > in the sink, and only release it (and give it back to the pool and let it be
> > > used upstream) after the next buffer is rendered.
> > 
> > I'm afraid I don't get it!
> > Do you mean I should keep pointer to the rendered buffer and unref the n-1 one?
> 
> I meant that your render function looks like this:
> 
> render(sink, buf) {
>   if (sink->last_buffer)
>     gst_buffer_unref(sink->last_buffer);
>   sink->last_buffer = gst_buffer_ref(buf);
>   do_dfb_stuff_to_render(buf);
> }

This will lead to reallocating a buffer for each frame whereas we could just keep switch between two preallocated buffers linked to the two buffers of the double buffer surface. (If I am clear :p)

I can manage to do thing at first if I flip the surface in the allocation code so that the next one is linked to the second buffer. Now that I am writing this I think I understand what is wrong with my code. When reallocating due to frame size change, I don't need to flip the surface cause it will be Flipped by the rendering process....  

I guess I'll have to add a test to see if we are playing or something like that

Actually, 2 frames are rendered using the same first buffer before another one is allocated...
what mechanism choose how many and when to allocate those buffers?
Can't we have the exact number of needed buffer being allocated from start?

Managed to get thing working! I'll try to share the patch tomorrow...

Eric, any news on this?

Hi,
sadly didn't have time to fix an issue where some time the bufferpool and the double buffered surface arn't in sync anymore, resulting in a blinking image. So I had to revert to a full image copy in the surface back buffer and then flip.

I'll try to find a nice solution but for now I'm working on a windows project using the D3videosink and trying to get rid of the problem with the window not being closed.

I'll share as soon as I have time to fix this properly

Eric, perhaps you could share your current solution even it's not perfect yet?

I understand it still fixes a crash in some cases?

Hi, 
I could never come up with a good solution for the double buffer usage. And now we are looking forward to use opengl sink and get rid of directfb.

If we do some more work on the directfb element in the future, wed'll be happy to share.

Cheers, 
Eric

Closing due to no activity in 4 years. If you get round to providing an updated patch, could you re-open the bug and provide a patch ? Thanks.