1712588 - Implement DMABufSurface::GetAsSourceSurface.

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

[Emilio Cobos Álvarez [:emilio]]

So that Canvas.drawImage(<video>) does draw an image as expected. I started writing some patches for this, but turns out it's a bit more complicated and I'm not too familiar with the media stack, so I'm going to post them here, but I don't think they're quite ready for review.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

So I think this works some of the time, other times by the time we try to read back the image, ReleaseDMABuf has been called already from the media decoder thread. Martin, do you know what is the best way to proceed here?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1712588 - Implement DMABufSurfaceRGBA::GetAsSourceSurface. r=stransky,rmader

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1712588 - Implement DMABufSurfaceYUV::GetAsSourceSurface. r=stransky

Mostly taken from CreateSourceSurfaceFromLockedMacIOSurface.

Depends on D115820

Comment 4

[Martin Stránský [:stransky] (ni? me)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

So I think this works some of the time, other times by the time we try to read back the image, ReleaseDMABuf has been called already from the media decoder thread. Martin, do you know what is the best way to proceed here?

When VA-API decoding is used, the DMABuf surfaces are re-used by decoder. VAAPI decoder allocates ~10-15 dmabuf surfaces internally and
exports them as decoded frames. We create DMABufSurfaceYUV wrapper over it but the dmabuf surface is owned by decoder. When we get the frame decoded into dmabuf surface, we export it as DMABufSurfaceYUV object to gecko/layout engine and use 'GlobalRef' functions to track its usage. As the dmabuf memory is shared across processes and we need to protect the data when any gecko component needs it. We use global lock for it, see 'GlobalRef' functions at DMABufSurface.h:

void GlobalRefCountCreate();
bool IsGlobalRefSet() const;
void GlobalRefAdd();
void GlobalRefRelease();

I expect the ReleaseDMABuf() is called here, right?
https://searchfox.org/mozilla-central/rev/08f063f4c89d270fd809fc0325b5a9000ae87d63/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp#716

And the surface state is tracked here:
https://searchfox.org/mozilla-central/rev/08f063f4c89d270fd809fc0325b5a9000ae87d63/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h#79

So if you want to keep the DMABufSurfaceYUV around you need to call GlobalRefAdd() on it and then use GlobalRefRelease() when you're finished.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Emilio, the patches look good to me.

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Moving to gfx as the code involved seems to be touched for gfx folks more often than media. Feel free to ping/move back if I've overlooked something.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Emilio, do we need to implement it? I see the WhatsApp is working now.
Thanks.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Okay, I managed to get a testcase at https://jsfiddle.net/onigetoc/v80khg1L/ so I'll look at it.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Yeah, whatsapp will work but the gif preview will be black.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Thanks. According to https://gitlab.gnome.org/GNOME/mutter/-/issues/1736 we should not map dmabuf directly by mmap (at least not the buffers we get from VA-API). So we need to map them as textures and use GL context fetch pixels.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1712588 Implement DMABufSurface::GetAsSourceSurface. r=sotaro,jgilbert

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1712588 [Linux] Don't release DMABuf surface from VideoFrameSurfaceVAAPI destructor r?alwu

Add a new param aForFrameRecycle to VideoFrameSurfaceVAAPI::ReleaseVAAPIData(). When it's set, release
all DMABuf surface internal data as the surface is going to be used for another frame.

When VideoFrameSurfaceVAAPI is deleted (for instance when decoder quits) underlying DMABuf surface may be still
used by layout code so don't release surface data in such case but wait to DMABuf surface destructor.

Depends on D116411

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1712588 Remove unnecessary asserts for gfx::Factory::CopyDataSourceSurface(), r?jgilbert

gfx::Factory::CopyDataSourceSurface() was updated so LOCAL_GL_RGBA readFormat and LOCAL_GL_UNSIGNED_BYTE readType is already supported.

Depends on D118302

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fceecd9a0981
[Linux] Don't release DMABuf surface from VideoFrameSurfaceVAAPI destructor r=alwu

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fceecd9a0981

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9f8ec6565f5
Remove unnecessary asserts for gfx::Factory::CopyDataSourceSurface(), r=jgilbert

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/001fa442c03e
Implement DMABufSurface::GetAsSourceSurface. r=sotaro,jgilbert

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f9f8ec6565f5
https://hg.mozilla.org/mozilla-central/rev/001fa442c03e

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:stransky, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

No, we don't need that.
Thanks.