| Message ID | 20200604185745.23568-1-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: uapi: cedrus: Fix decoding interlaced H264 content | expand |
Hi Jernej, On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Currently H264 interlaced content it's not properly decoded on Cedrus. > There are two reasons for this: > 1. slice parameters control doesn't provide enough information > 2. bug in frame list construction in Cedrus driver > > As described in commit message in patch 1, references stored in > reference lists should tell if reference targets top or bottom field. > However, this information is currently not provided. Patch 1 adds > it in form of flags which are set for each reference. Patch 2 then > uses those flags in Cedrus driver. > > Frame list construction is fixed in patch 3. > > This solution was extensively tested using Kodi on LibreELEC with A64, > H3, H5 and H6 SoCs in slightly different form (flags were transmitted > in MSB bits in index). > So, if I understand correctly the field needs to be passed per-reference, and the current per-DPB entry is not good? If you could point at the userspace code for this, it would be interesting to take a look. > Note: I'm not 100% sure if flags for both, top and bottom fields are > needed. Any input here would be welcome. > Given enum v4l2_field is already part of the uAPI, perhaps it makes sense to just reuse that for the field type? Maybe it's an overkill, but it would make sense to reuse the concepts and types that already exist. We can still add a reserved field to make this new reference type extensive. Thanks, Ezequiel > Please take a look. > > Best regards, > Jernej > > Jernej Skrabec (3): > media: uapi: h264: update reference lists > media: cedrus: h264: Properly configure reference field > media: cedrus: h264: Fix frame list construction > > .../media/v4l/ext-ctrls-codec.rst | 40 ++++++++++++++++++- > .../staging/media/sunxi/cedrus/cedrus_h264.c | 27 +++++++------ > include/media/h264-ctrls.h | 12 +++++- > 3 files changed, 62 insertions(+), 17 deletions(-) > > -- > 2.27.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Le samedi 06 juin 2020 à 09:46 -0300, Ezequiel Garcia a écrit : > Hi Jernej, > > On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Currently H264 interlaced content it's not properly decoded on Cedrus. > > There are two reasons for this: > > 1. slice parameters control doesn't provide enough information > > 2. bug in frame list construction in Cedrus driver > > > > As described in commit message in patch 1, references stored in > > reference lists should tell if reference targets top or bottom field. > > However, this information is currently not provided. Patch 1 adds > > it in form of flags which are set for each reference. Patch 2 then > > uses those flags in Cedrus driver. > > > > Frame list construction is fixed in patch 3. > > > > This solution was extensively tested using Kodi on LibreELEC with A64, > > H3, H5 and H6 SoCs in slightly different form (flags were transmitted > > in MSB bits in index). > > > > So, if I understand correctly the field needs to be passed per-reference, > and the current per-DPB entry is not good? For interlaced content we reference fields separately. That's the reason there is 32 entries in l0/l1. Though, as we decode both fields in the same buffer (interleaved), the DPB indice is not sufficient to inform the decoder what we are referencing. Understand that a top field can be used to decode the next bottom field. This even make sense as they are closer on the capture timeline. This covers slice based decoders. The flags to indicate presence of top/bottom fields in DPB array seems only useful to frame base decoders. This is so that decoder can avoid using lost fields when constructing it's own l0/l1 internally. > > If you could point at the userspace code for this, it would be interesting > to take a look. > > > Note: I'm not 100% sure if flags for both, top and bottom fields are > > needed. Any input here would be welcome. > > > > Given enum v4l2_field is already part of the uAPI, perhaps it makes > sense to just reuse that for the field type? Maybe it's an overkill, > but it would make sense to reuse the concepts and types that > already exist. > > We can still add a reserved field to make this new reference type > extensive. I think it's fine to create new flag for this, as your solution would require extending a reference to 24bit (this patch extend to 16bits). Though indeed, we could combine frame and TOP and reserve more bits for future use. > > Thanks, > Ezequiel > > > > Please take a look. > > > > Best regards, > > Jernej > > > > Jernej Skrabec (3): > > media: uapi: h264: update reference lists > > media: cedrus: h264: Properly configure reference field > > media: cedrus: h264: Fix frame list construction > > > > .../media/v4l/ext-ctrls-codec.rst | 40 ++++++++++++++++++- > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 27 +++++++------ > > include/media/h264-ctrls.h | 12 +++++- > > 3 files changed, 62 insertions(+), 17 deletions(-) > > > > -- > > 2.27.0 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Le dimanche 07 juin 2020 à 16:21 -0400, Nicolas Dufresne a écrit : > Le samedi 06 juin 2020 à 09:46 -0300, Ezequiel Garcia a écrit : > > Hi Jernej, > > > > On Thu, 4 Jun 2020 at 15:55, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > > Currently H264 interlaced content it's not properly decoded on Cedrus. > > > There are two reasons for this: > > > 1. slice parameters control doesn't provide enough information > > > 2. bug in frame list construction in Cedrus driver > > > > > > As described in commit message in patch 1, references stored in > > > reference lists should tell if reference targets top or bottom field. > > > However, this information is currently not provided. Patch 1 adds > > > it in form of flags which are set for each reference. Patch 2 then > > > uses those flags in Cedrus driver. > > > > > > Frame list construction is fixed in patch 3. > > > > > > This solution was extensively tested using Kodi on LibreELEC with A64, > > > H3, H5 and H6 SoCs in slightly different form (flags were transmitted > > > in MSB bits in index). > > > > > > > So, if I understand correctly the field needs to be passed per-reference, > > and the current per-DPB entry is not good? > > For interlaced content we reference fields separately. That's the > reason there is 32 entries in l0/l1. Though, as we decode both fields > in the same buffer (interleaved), the DPB indice is not sufficient to > inform the decoder what we are referencing. Understand that a top field > can be used to decode the next bottom field. This even make sense as > they are closer on the capture timeline. This covers slice based > decoders. > > The flags to indicate presence of top/bottom fields in DPB array seems > only useful to frame base decoders. This is so that decoder can avoid > using lost fields when constructing it's own l0/l1 internally. > > > If you could point at the userspace code for this, it would be interesting > > to take a look. > > > > > Note: I'm not 100% sure if flags for both, top and bottom fields are > > > needed. Any input here would be welcome. > > > > > > > Given enum v4l2_field is already part of the uAPI, perhaps it makes > > sense to just reuse that for the field type? Maybe it's an overkill, > > but it would make sense to reuse the concepts and types that > > already exist. > > > > We can still add a reserved field to make this new reference type > > extensive. > > I think it's fine to create new flag for this, as your solution would > require extending a reference to 24bit (this patch extend to 16bits). > Though indeed, we could combine frame and TOP and reserve more bits for > future use. Sorry for the oversight, the flags is 16bits, so we already use 24bits. But looking at "enum v4l2_field", which is not a flag, seems like a miss-fit. It would create such a confusion, as V4L2_FIELD_SEQ_TB/BT can still be used with this interface, even though we still need to say if we reference TOP or BOTTOM. Only V4L2_FIELD_ALTERNATE is not supported. But as you can see, "enum v4l2_fiel" is really meant to describe the layout of the interleaved frame rather then signalling a field directly. > > > Thanks, > > Ezequiel > > > > > > > Please take a look. > > > > > > Best regards, > > > Jernej > > > > > > Jernej Skrabec (3): > > > media: uapi: h264: update reference lists > > > media: cedrus: h264: Properly configure reference field > > > media: cedrus: h264: Fix frame list construction > > > > > > .../media/v4l/ext-ctrls-codec.rst | 40 ++++++++++++++++++- > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 27 +++++++------ > > > include/media/h264-ctrls.h | 12 +++++- > > > 3 files changed, 62 insertions(+), 17 deletions(-) > > > > > > -- > > > 2.27.0 > > > > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel