| Message ID | 20231206144822.3194663-1-devarsht@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Ryan Eatmon |
| Headers | show |
| Series | [meta-arago,kirkstone] receipes-multimedia: gstreamer: Add patches for AM62P | expand |
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"
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"
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"
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"
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"
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"
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(+)