[meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[Devarsh Thakkar]

Add patches to prefer contiguous video format and supporting
dmabuf-import mode for video decoder.

This is to support use-cases involving video codec with other components
viz. GPU, Camera e.t.c.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 .../gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend       | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
index a36c9760..e14a3c93 100644
--- a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
+++ b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
@@ -20,4 +20,8 @@ SRC_URI:append:am62axx = " \
     file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
 "
 
+SRC_URI:append:am62pxx = " \
+    file://0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch \
+    file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
+"
 PR:append = ".arago0"
-- 
2.34.1

Re: [meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[Andrew Davis]

On 12/6/23 8:48 AM, Devarsh Thakkar wrote:
> Add patches to prefer contiguous video format and supporting
> dmabuf-import mode for video decoder.
> 
> This is to support use-cases involving video codec with other components
> viz. GPU, Camera e.t.c.
> 

Why does this "support use-cases"? What is wrong with the default formats?
Are these posted upstream to upstream gstreamer, if not why? Or should we
instead fix the consuming software to handle the default non-contiguous
formats better..

Andrew

> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>   .../gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend       | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
> index a36c9760..e14a3c93 100644
> --- a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
> +++ b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
> @@ -20,4 +20,8 @@ SRC_URI:append:am62axx = " \
>       file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>   "
>   
> +SRC_URI:append:am62pxx = " \
> +    file://0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch \
> +    file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
> +"
>   PR:append = ".arago0"

Re: [meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[Devarsh Thakkar]

Hi Andrew,

Thanks for the review.

On 06/12/23 20:41, Andrew Davis wrote:
> On 12/6/23 8:48 AM, Devarsh Thakkar wrote:
>> Add patches to prefer contiguous video format and supporting
>> dmabuf-import mode for video decoder.
>>
>> This is to support use-cases involving video codec with other components
>> viz. GPU, Camera e.t.c.
>>
> 
> Why does this "support use-cases"? What is wrong with the default formats?

For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
I think we have certain blocks such as ISP, Camera seem to only support
contigous formats which doesn't go well with gstreamer v4l2 which prefers
non-contigous by default and we fail at negotiation.

For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
I think this is a local patch which complements a patch already made available
in driver, this to support dmabuf import for the decoder.

> Are these posted upstream to upstream gstreamer, if not why? Or should we
> instead fix the consuming software to handle the default non-contiguous
> formats better..
> 
For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :

The crux of the issue is that unlike v4l2 gstreamer has only single GST video
format for both NV12 and NV12M (non-contigous) and it prefers to advertise
latter during caps negotiation. I think there was a proposal long back to have
separate GST video formats for NV12 and NV12M but that didn't converge,
perhaps they wanted to support this with a VideoMeta based scheme as per logs [0].

Also I see other vendors also carrying similar patch like this locally.

For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
There again is discussion around this to support this in upstream but the
upstream patch is not yet merged [1].

Anyways, I don't see any harm in applying these patches if they are helping
achieve use-cases and I see they are already getting applied for other platforms.

[0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/414
[1] :
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4550/diffs?commit_id=b12eaf65f489bab50917e22242758e000d69df58

Regards
Devarsh

> Andrew
> 
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>>   .../gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend       | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git
>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>> index a36c9760..e14a3c93 100644
>> ---
>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>> +++
>> b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>> @@ -20,4 +20,8 @@ SRC_URI:append:am62axx = " \
>>       file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>   "
>>   +SRC_URI:append:am62pxx = " \
>> +    file://0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch \
>> +    file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>> +"
>>   PR:append = ".arago0"

Re: [meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[Rahul T R]

On 06/12/23 21:26, Devarsh Thakkar wrote:

> Hi Andrew,
>
> Thanks for the review.
>
> On 06/12/23 20:41, Andrew Davis wrote:
>> On 12/6/23 8:48 AM, Devarsh Thakkar wrote:
>>> Add patches to prefer contiguous video format and supporting
>>> dmabuf-import mode for video decoder.
>>>
>>> This is to support use-cases involving video codec with other components
>>> viz. GPU, Camera e.t.c.
>>>
>> Why does this "support use-cases"? What is wrong with the default formats?
> For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
> I think we have certain blocks such as ISP, Camera seem to only support
> contigous formats which doesn't go well with gstreamer v4l2 which prefers
> non-contigous by default and we fail at negotiation.
>
> For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
> I think this is a local patch which complements a patch already made available
> in driver, this to support dmabuf import for the decoder.
>
>> Are these posted upstream to upstream gstreamer, if not why? Or should we
>> instead fix the consuming software to handle the default non-contiguous
>> formats better..
>>
> For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
>
> The crux of the issue is that unlike v4l2 gstreamer has only single GST video
> format for both NV12 and NV12M (non-contigous) and it prefers to advertise
> latter during caps negotiation. I think there was a proposal long back to have
> separate GST video formats for NV12 and NV12M but that didn't converge,
> perhaps they wanted to support this with a VideoMeta based scheme as per logs [0].
>
> Also I see other vendors also carrying similar patch like this locally.
>
> For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
> There again is discussion around this to support this in upstream but the
> upstream patch is not yet merged [1].
>
> Anyways, I don't see any harm in applying these patches if they are helping
> achieve use-cases and I see they are already getting applied for other platforms.
>
> [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/414
> [1] :
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4550/diffs?commit_id=b12eaf65f489bab50917e22242758e000d69df58
>
> Regards
> Devarsh
Reviewed-by: Rahul T R <r-ravikumar@ti.com>

Regards
Rahul T R
>> Andrew
>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
>>>    .../gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend       | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git
>>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>> index a36c9760..e14a3c93 100644
>>> ---
>>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>> +++
>>> b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>> @@ -20,4 +20,8 @@ SRC_URI:append:am62axx = " \
>>>        file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>>    "
>>>    +SRC_URI:append:am62pxx = " \
>>> +    file://0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch \
>>> +    file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>> +"
>>>    PR:append = ".arago0"

Re: [meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[Andrew Davis]

On 12/6/23 9:56 AM, Devarsh Thakkar wrote:
> Hi Andrew,
> 
> Thanks for the review.
> 
> On 06/12/23 20:41, Andrew Davis wrote:
>> On 12/6/23 8:48 AM, Devarsh Thakkar wrote:
>>> Add patches to prefer contiguous video format and supporting
>>> dmabuf-import mode for video decoder.
>>>
>>> This is to support use-cases involving video codec with other components
>>> viz. GPU, Camera e.t.c.
>>>
>>
>> Why does this "support use-cases"? What is wrong with the default formats?
> 
> For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
> I think we have certain blocks such as ISP, Camera seem to only support
> contigous formats which doesn't go well with gstreamer v4l2 which prefers
> non-contigous by default and we fail at negotiation.
> 
> For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
> I think this is a local patch which complements a patch already made available
> in driver, this to support dmabuf import for the decoder.
> 
>> Are these posted upstream to upstream gstreamer, if not why? Or should we
>> instead fix the consuming software to handle the default non-contiguous
>> formats better..
>>
> For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
> 
> The crux of the issue is that unlike v4l2 gstreamer has only single GST video
> format for both NV12 and NV12M (non-contigous) and it prefers to advertise
> latter during caps negotiation. I think there was a proposal long back to have
> separate GST video formats for NV12 and NV12M but that didn't converge,
> perhaps they wanted to support this with a VideoMeta based scheme as per logs [0].
> 
> Also I see other vendors also carrying similar patch like this locally.
> 
> For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
> There again is discussion around this to support this in upstream but the
> upstream patch is not yet merged [1].
> 
> Anyways, I don't see any harm in applying these patches if they are helping
> achieve use-cases and I see they are already getting applied for other platforms.
> 

This is a lot of good information, it should be in the commit message.

I do not see any harm either, this was not a NAK. But we do need to remember to
work on fixing our drivers/applications to better handle NV12M (seems like
it will remain the default in gstreamer). Otherwise you will have to support this
hack forever, and for every distro (we have Debian now, I don't see you updating
gstreamer.deb for the same..).

Andrew

> [0] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/414
> [1] :
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4550/diffs?commit_id=b12eaf65f489bab50917e22242758e000d69df58
> 
> Regards
> Devarsh
> 
>> Andrew
>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
>>>    .../gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend       | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git
>>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>> index a36c9760..e14a3c93 100644
>>> ---
>>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>> +++
>>> b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>> @@ -20,4 +20,8 @@ SRC_URI:append:am62axx = " \
>>>        file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>>    "
>>>    +SRC_URI:append:am62pxx = " \
>>> +    file://0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch \
>>> +    file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>> +"
>>>    PR:append = ".arago0"

Re: [meta-arago] [kirkstone] [PATCH] receipes-multimedia: gstreamer: Add patches for AM62P

[Ryan Eatmon]

On 12/6/2023 11:03 AM, Andrew Davis wrote:
> On 12/6/23 9:56 AM, Devarsh Thakkar wrote:
>> Hi Andrew,
>>
>> Thanks for the review.
>>
>> On 06/12/23 20:41, Andrew Davis wrote:
>>> On 12/6/23 8:48 AM, Devarsh Thakkar wrote:
>>>> Add patches to prefer contiguous video format and supporting
>>>> dmabuf-import mode for video decoder.
>>>>
>>>> This is to support use-cases involving video codec with other 
>>>> components
>>>> viz. GPU, Camera e.t.c.
>>>>
>>>
>>> Why does this "support use-cases"? What is wrong with the default 
>>> formats?
>>
>> For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
>> I think we have certain blocks such as ISP, Camera seem to only support
>> contigous formats which doesn't go well with gstreamer v4l2 which prefers
>> non-contigous by default and we fail at negotiation.
>>
>> For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
>> I think this is a local patch which complements a patch already made 
>> available
>> in driver, this to support dmabuf import for the decoder.
>>
>>> Are these posted upstream to upstream gstreamer, if not why? Or 
>>> should we
>>> instead fix the consuming software to handle the default non-contiguous
>>> formats better..
>>>
>> For 0002-v4l2-Give-preference-to-contiguous-format-if-support.patch :
>>
>> The crux of the issue is that unlike v4l2 gstreamer has only single 
>> GST video
>> format for both NV12 and NV12M (non-contigous) and it prefers to 
>> advertise
>> latter during caps negotiation. I think there was a proposal long back 
>> to have
>> separate GST video formats for NV12 and NV12M but that didn't converge,
>> perhaps they wanted to support this with a VideoMeta based scheme as 
>> per logs [0].
>>
>> Also I see other vendors also carrying similar patch like this locally.
>>
>> For 0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch :
>> There again is discussion around this to support this in upstream but the
>> upstream patch is not yet merged [1].
>>
>> Anyways, I don't see any harm in applying these patches if they are 
>> helping
>> achieve use-cases and I see they are already getting applied for other 
>> platforms.
>>
> 
> This is a lot of good information, it should be in the commit message.

Devarsh, can you put together a v2 with the above information in the 
commit message?  History like this is very helpful to keep track of so 
that we do not lose it to the passage of time.


> I do not see any harm either, this was not a NAK. But we do need to 
> remember to
> work on fixing our drivers/applications to better handle NV12M (seems like
> it will remain the default in gstreamer). Otherwise you will have to 
> support this
> hack forever, and for every distro (we have Debian now, I don't see you 
> updating
> gstreamer.deb for the same..).
> 
> Andrew
> 
>> [0] 
>> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/414
>> [1] :
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4550/diffs?commit_id=b12eaf65f489bab50917e22242758e000d69df58
>>
>> Regards
>> Devarsh
>>
>>> Andrew
>>>
>>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>>> ---
>>>>    .../gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend       | 4 
>>>> ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git
>>>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>>> index a36c9760..e14a3c93 100644
>>>> ---
>>>> a/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>>> +++
>>>> b/meta-arago-extras/recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.20.%.bbappend
>>>> @@ -20,4 +20,8 @@ SRC_URI:append:am62axx = " \
>>>>        
>>>> file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>>>    "
>>>>    +SRC_URI:append:am62pxx = " \
>>>> +    file://0001-v4l2-Changes-for-DMA-Buf-import-j721s2.patch \
>>>> +    
>>>> file://0002-v4l2-Give-preference-to-contiguous-format-if-support.patch \
>>>> +"
>>>>    PR:append = ".arago0"

-- 
Ryan Eatmon                reatmon@ti.com
-----------------------------------------
Texas Instruments, Inc.  -  LCPD  -  MGTS