From: Nicolas Dufresne nicolas.dufresne@collabora.com
The CODA960 manual states that ASO/FMO features of baseline are not supported, so for this reason this driver should only report constrained baseline support.
This fixes negotiation issue with constrained baseline content on GStreamer 1.17.1.
Cc: stable@vger.kernel.org Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls") Signed-off-by: Nicolas Dufresne nicolas.dufresne@collabora.com Signed-off-by: Ezequiel Garcia ezequiel@collabora.com --- drivers/media/platform/coda/coda-common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 3ab3d976d8ca..c641d1608825 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx) V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0); v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE, - V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0, - V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE); + V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0, + V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE); if (ctx->dev->devtype->product == CODA_HX4 || ctx->dev->devtype->product == CODA_7541) { v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx) ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE, V4L2_MPEG_VIDEO_H264_PROFILE_HIGH, - ~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) | + ~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) | (1 << V4L2_MPEG_VIDEO_H264_PROFILE_MAIN) | (1 << V4L2_MPEG_VIDEO_H264_PROFILE_HIGH)), V4L2_MPEG_VIDEO_H264_PROFILE_HIGH);
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them. The constraints on frame size and pixel formats already cover the limitation.
This fixes negotiation of level on GStreamer 1.17.1.
Cc: stable@vger.kernel.org Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls") Signed-off-by: Nicolas Dufresne nicolas.dufresne@collabora.com Signed-off-by: Ezequiel Garcia ezequiel@collabora.com --- drivers/media/platform/coda/coda-common.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index c641d1608825..d0fc573d6ed9 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2355,7 +2355,10 @@ static void coda_encode_ctrls(struct coda_ctx *ctx) (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_0) | (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_1) | (1 << V4L2_MPEG_VIDEO_H264_LEVEL_3_2) | - (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0)), + (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_0) | + (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_1) | + (1 << V4L2_MPEG_VIDEO_H264_LEVEL_4_2) | + (1 << V4L2_MPEG_VIDEO_H264_LEVEL_5_0)), V4L2_MPEG_VIDEO_H264_LEVEL_4_0); } v4l2_ctrl_new_std(&ctx->ctrls, &coda_ctrl_ops, @@ -2428,7 +2431,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx) ctx->dev->devtype->product == CODA_7541) max = V4L2_MPEG_VIDEO_H264_LEVEL_4_0; else if (ctx->dev->devtype->product == CODA_960) - max = V4L2_MPEG_VIDEO_H264_LEVEL_4_1; + max = V4L2_MPEG_VIDEO_H264_LEVEL_5_0; else return; ctx->h264_level_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls,
Hi Ezequiel, Nicolas,
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them.
Could you clarify this? As far as I understand the hardware supports maximum frame size requirement for up to level 4.2 (8704 macroblocks), but not 5.0, and at least the implementation on i.MX6 does not support the max encoding speed requirements for levels 4.1 and higher.
I don't think the firmware ever produces any output with a level higher than 4.0 either, so what is the purpose of pretending otherwise?
regards Philipp
Le vendredi 17 juillet 2020 à 09:48 +0200, Philipp Zabel a écrit :
Hi Ezequiel, Nicolas,
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them.
Could you clarify this? As far as I understand the hardware supports maximum frame size requirement for up to level 4.2 (8704 macroblocks), but not 5.0, and at least the implementation on i.MX6 does not support the max encoding speed requirements for levels 4.1 and higher.
I don't think the firmware ever produces any output with a level higher than 4.0 either, so what is the purpose of pretending otherwise?
The level is a combination of 3 contraints, frame size, raw bitrate and encoded bitrate. We have a streams here the decode just fine, that reaches 5.0 for the endoded bitrate, but is near 4.0 for everything else. This streams works just fine with the 960. I think the risk with this patch is that it now allow a stream to underperform in raw bitrate, but that can be controlled otherwise by the frame interval, so there is no need to limit it through levels. I could be wrong.
But in public domain [0], Chips&Media seems to claim 4.2 decode, 4.0 encode. So yes, claiming 5.0 is off track, we will reduce this to 4.2 in v2.
[0] https://www.chipsnmedia.com/fullhd
Considering how buggy and inconcistent this is going to be in decoder drivers, I'm tempted to just drop that restriction in GStreamer v4l2 decoders (was added by Philippe Normand from Igalia). Specially the bitrate limits, since it is quite clear from testing that this limits is only related to real-time performance, and that offline decoding should still be possible. Meanwhile, the driver should still advertise 4.1 and 4.2 decoding. But we should check the decoding/encoding levels are actually not the same, that I haven't checked, the code is a bit ... kindly said ... hairy.
regards Philipp
Hi Nicolas,
On Fri, 2020-07-17 at 11:50 -0400, Nicolas Dufresne wrote:
Le vendredi 17 juillet 2020 à 09:48 +0200, Philipp Zabel a écrit :
Hi Ezequiel, Nicolas,
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them.
Could you clarify this? As far as I understand the hardware supports maximum frame size requirement for up to level 4.2 (8704 macroblocks), but not 5.0, and at least the implementation on i.MX6 does not support the max encoding speed requirements for levels 4.1 and higher.
I don't think the firmware ever produces any output with a level higher than 4.0 either, so what is the purpose of pretending otherwise?
I didn't see the decoder change, ^ this was just referring to the encoder level.
The level is a combination of 3 contraints, frame size, raw bitrate and encoded bitrate. We have a streams here the decode just fine, that reaches 5.0 for the endoded bitrate, but is near 4.0 for everything else. This streams works just fine with the 960.
You are right that the decoder, depending on the individual stream, may well be capable of playing back a higher level than officially supported. It is just not guaranteed that any stream of that unsupported level can play back smoothly.
I suppose on i.MX6 the bottleneck is more likely to be the macroblock processing rate than the encoded bitrate, especially if the memory bus is under load. I'm not sure we should increase the advertised level unless we can reach required MB/s as well. That being said, there is a kernel option in the i.MX6 vendor kernel that disables CPU and bus frequency scaling, keeps the SoC voltage high, and overclocks the VPU to 352 MHz. So there might be some headroom left to actually support this.
I think the risk with this patch is that it now allow a stream to underperform in raw bitrate, but that can be controlled otherwise by the frame interval, so there is no need to limit it through levels. I could be wrong.
But in public domain [0], Chips&Media seems to claim 4.2 decode, 4.0 encode. So yes, claiming 5.0 is off track, we will reduce this to 4.2 in v2.
I'm not sure the CODA960 VPU on i.MX6 at 264 MHz is as capable as the CODA966 mentioned on their website.
Considering how buggy and inconcistent this is going to be in decoder drivers, I'm tempted to just drop that restriction in GStreamer v4l2 decoders (was added by Philippe Normand from Igalia). Specially the bitrate limits, since it is quite clear from testing that this limits is only related to real-time performance, and that offline decoding should still be possible. Meanwhile, the driver should still advertise 4.1 and 4.2 decoding. But we should check the decoding/encoding levels are actually not the same, that I haven't checked, the code is a bit ... kindly said ... hairy.
I think negotiation is important for sources that can provide multiple levels, to choose the right level for the decoder. If there is a given stream with a fixed level, it might indeed be better to not fail negotiation (maybe have a warning instead) and just hope for the best, as for some streams it might just work.
regards Philipp
Le lundi 20 juillet 2020 à 10:31 +0200, Philipp Zabel a écrit :
Considering how buggy and inconcistent this is going to be in decoder drivers, I'm tempted to just drop that restriction in GStreamer v4l2 decoders (was added by Philippe Normand from Igalia). Specially the bitrate limits, since it is quite clear from testing that this limits is only related to real-time performance, and that offline decoding should still be possible. Meanwhile, the driver should still advertise 4.1 and 4.2 decoding. But we should check the decoding/encoding levels are actually not the same, that I haven't checked, the code is a bit ... kindly said ... hairy.
I think negotiation is important for sources that can provide multiple levels, to choose the right level for the decoder. If there is a given stream with a fixed level, it might indeed be better to not fail negotiation (maybe have a warning instead) and just hope for the best, as for some streams it might just work.
Yes, agreed, but I didn't want to use the linux-media list to discuss GStreamer designs. I have a soltion for that, I'll send a MR and will CC you. For the general idea, I'll try and keep the levels as "preferred" capabilities while allowing any levels. Same mechanism used to proposed an unscaled display resolution, even though scaling might be supported.
Nicolas
Le vendredi 17 juillet 2020 à 09:48 +0200, Philipp Zabel a écrit :
Hi Ezequiel, Nicolas,
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them.
Could you clarify this? As far as I understand the hardware supports maximum frame size requirement for up to level 4.2 (8704 macroblocks), but not 5.0, and at least the implementation on i.MX6 does not support the max encoding speed requirements for levels 4.1 and higher.
I don't think the firmware ever produces any output with a level higher than 4.0 either, so what is the purpose of pretending otherwise?
Nothing is very explicit in the user manual, they speak in term of resolution and framerate. They claim 1080p 30fps for encoding, and 1080p 60fps for decoding. For the encoder, there is an auto selection for the level, and the documentation is maxed to 4.0, and so I would agree that 4.0 is the max encoding level. Wikipedia also list "1,920×1, 080@30.1 (4)" so 1080p30 with 4 frame references as being an example of 4.0 maximum. So V2 of this patchset should make sure that for the encoder this stays there.
On the decoding side, what I found is that there is an error bit indicator called LEVELID (bit 19) that indicates that SPS level_idc wasn't accepted. The error is described as "Supported up to 51.". So basically there is some extra contraints that least to 4.2 as you describe, and above 5.1 is an hard failure. That imho creates a grey- zone. If we think of DASH/HLS, the information usually comes with Resolution/Framerate/Codec/Profile/Level, and in this context, you can enable 5.1 safely assuming the Resolition/Framerate/Profile are already verified. But if you only wanted to use the level, then you could prefer the driver to expose a max of 4.2.
So do you have an opinion on the way forward ? Personally I like the idea of giving the list of level_idc that won't cause the parser to reject it, and leave it to the user to validate the Resolution/Framerate seperatly, we have the V4L2 API for that. Let me know, as we'll use that for V2.
regards Philipp
Le lundi 20 juillet 2020 à 12:09 -0400, Nicolas Dufresne a écrit :
Le vendredi 17 juillet 2020 à 09:48 +0200, Philipp Zabel a écrit :
Hi Ezequiel, Nicolas,
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them.
Could you clarify this? As far as I understand the hardware supports maximum frame size requirement for up to level 4.2 (8704 macroblocks), but not 5.0, and at least the implementation on i.MX6 does not support the max encoding speed requirements for levels 4.1 and higher.
I don't think the firmware ever produces any output with a level higher than 4.0 either, so what is the purpose of pretending otherwise?
Nothing is very explicit in the user manual, they speak in term of resolution and framerate. They claim 1080p 30fps for encoding, and 1080p 60fps for decoding. For the encoder, there is an auto selection for the level, and the documentation is maxed to 4.0, and so I would agree that 4.0 is the max encoding level. Wikipedia also list "1,920×1, 080@30.1 (4)" so 1080p30 with 4 frame references as being an example of 4.0 maximum. So V2 of this patchset should make sure that for the encoder this stays there.
On the decoding side, what I found is that there is an error bit indicator called LEVELID (bit 19) that indicates that SPS level_idc wasn't accepted. The error is described as "Supported up to 51.". So basically there is some extra contraints that least to 4.2 as you describe, and above 5.1 is an hard failure. That imho creates a grey- zone. If we think of DASH/HLS, the information usually comes with Resolution/Framerate/Codec/Profile/Level, and in this context, you can enable 5.1 safely assuming the Resolition/Framerate/Profile are already verified. But if you only wanted to use the level, then you could prefer the driver to expose a max of 4.2.
So do you have an opinion on the way forward ? Personally I like the idea of giving the list of level_idc that won't cause the parser to reject it, and leave it to the user to validate the Resolution/Framerate seperatly, we have the V4L2 API for that. Let me know, as we'll use that for V2.
Dropping some extra information I received, the CODA960 is designed to run at 352MHz, but on IMX.6 we run it at 264MHz. Means that CODA960 on IMX.6 is special, it under-perform the spec by 25%, so it's more of a 1080p45 decoder. At this level of variation, it sounds like that should endup in per SoC, since you could design an IMX.6 board with better heat dissipation that could support 352.
regards Philipp
Hi,
[adding Stanimir for some insight, venus also has a decoder h.264 level control]
On Mon, 2020-07-20 at 12:09 -0400, Nicolas Dufresne wrote:
Le vendredi 17 juillet 2020 à 09:48 +0200, Philipp Zabel a écrit :
Hi Ezequiel, Nicolas,
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
This add H264 level 4.1, 4.2 and 5.0 to the list of supported formats. While the hardware does not fully support these levels, it do support most of them.
Could you clarify this? As far as I understand the hardware supports maximum frame size requirement for up to level 4.2 (8704 macroblocks), but not 5.0, and at least the implementation on i.MX6 does not support the max encoding speed requirements for levels 4.1 and higher.
[...]
So do you have an opinion on the way forward ? Personally I like the idea of giving the list of level_idc that won't cause the parser to reject it, and leave it to the user to validate the Resolution/Framerate seperatly, we have the V4L2 API for that. Let me know, as we'll use that for V2.
My opinion was that for decoders the possible values of the V4L2_CID_MPEG_VIDEO_H264_LEVEL control should reflect the h.264 levels that the decoder can actually decode in real-time. Otherwise we are effectively ignoring the MaxMBPS and MaxBR properties of the level, which makes the control useless for negotiation.
But I am beginning to think that I am wrong. The level control is set to the level of the stream after parsing the stream header, so arguably it must be possible to set it to anything that can be decoded at all, real- time or not.
Further, the documentation says nothing about this. It doesn't even mention decoders:
``V4L2_CID_MPEG_VIDEO_H264_LEVEL`` (enum)
enum v4l2_mpeg_video_h264_level - The level information for the H264 video elementary stream. Applicable to the H264 encoder. Possible values are:
[...]
So at the moment I would tend towards expanding the list of supported formats to 4.2, even though we can't decoder > 4.0 in real-time on i.MX6.
Should we add a note to the V4L2_CID_MPEG_VIDEO_H264_LEVEL documentation that this is applicable to H264 decoders as well, set by the decoder after parsing the SPS, and that the list of levels this control can be set to does not guarantee real-time capability at each level?
regards Philipp
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
The CODA960 manual states that ASO/FMO features of baseline are not supported, so for this reason this driver should only report constrained baseline support.
I know the encoder doesn't support this, but is this also true of the decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder support for both baseline profile and constrained base line profile.
This fixes negotiation issue with constrained baseline content on GStreamer 1.17.1.
Cc: stable@vger.kernel.org Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls") Signed-off-by: Nicolas Dufresne nicolas.dufresne@collabora.com Signed-off-by: Ezequiel Garcia ezequiel@collabora.com
drivers/media/platform/coda/coda-common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 3ab3d976d8ca..c641d1608825 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx) V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0); v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);
Encoder support is listed as baseline, not constrained baseline, in the manual, but the SPS NALs produced by the encoder start with: 00 00 00 01 67 42 40 ^ so that is profile_idc=66, constraint_set1_flag==1, constrained baseline indeed. I think this change is correct.
if (ctx->dev->devtype->product == CODA_HX4 || ctx->dev->devtype->product == CODA_7541) { v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx) ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE, V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |
I'm not sure about this one.
regards Philipp
Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
The CODA960 manual states that ASO/FMO features of baseline are not supported, so for this reason this driver should only report constrained baseline support.
I know the encoder doesn't support this, but is this also true of the decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder support for both baseline profile and constrained base line profile.
Hmm, double checking, you are right this is documented in the encoding tools sections, not the decoding. But there is extra buffers that need to be passed for ASO/FMO to work, I greatly doubt you have ever tested it. This is not supported by GStreamer parser, or FFMPEG parsers either. Again, we need to make sure in V2 that encoding and decoding capabilities are well seperated.
As for advertising ASO/FMO, I can leave it there, but be aware I won't be testing it. I can provide you links to streams if you care (they are publicly accessible throught the ITU conformance streams published by the ITU). But as for GStreamer and FFMPEG, this is not supported anyway.
This fixes negotiation issue with constrained baseline content on GStreamer 1.17.1.
Cc: stable@vger.kernel.org Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls") Signed-off-by: Nicolas Dufresne nicolas.dufresne@collabora.com Signed-off-by: Ezequiel Garcia ezequiel@collabora.com
drivers/media/platform/coda/coda-common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 3ab3d976d8ca..c641d1608825 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx) V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0); v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);Encoder support is listed as baseline, not constrained baseline, in the manual, but the SPS NALs produced by the encoder start with: 00 00 00 01 67 42 40 ^ so that is profile_idc=66, constraint_set1_flag==1, constrained baseline indeed. I think this change is correct.
if (ctx->dev->devtype->product == CODA_HX4 || ctx->dev->devtype->product == CODA_7541) { v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx) ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE, V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |I'm not sure about this one.
regards Philipp
On Fri, 2020-07-17 at 11:56 -0400, Nicolas Dufresne wrote:
Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
The CODA960 manual states that ASO/FMO features of baseline are not supported, so for this reason this driver should only report constrained baseline support.
I know the encoder doesn't support this, but is this also true of the decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder support for both baseline profile and constrained base line profile.
Hmm, double checking, you are right this is documented in the encoding tools sections, not the decoding. But there is extra buffers that need to be passed for ASO/FMO to work, I greatly doubt you have ever tested it.
And you are correct, I don't think I use any test streams that have ASO/FMO enabled.
This is not supported by GStreamer parser, or FFMPEG parsers either. Again, we need to make sure in V2 that encoding and decoding capabilities are well seperated.
As for advertising ASO/FMO, I can leave it there, but be aware I won't be testing it. I can provide you links to streams if you care (they are publicly accessible throught the ITU conformance streams published by the ITU).
That would be welcome.
But as for GStreamer and FFMPEG, this is not supported anyway.
Ok, how about changing the commit message to say that this is unsupported for the encoder and untested for the decoder because there is no userspace support?
regards Philipp
Le lundi 20 juillet 2020 à 10:40 +0200, Philipp Zabel a écrit :
On Fri, 2020-07-17 at 11:56 -0400, Nicolas Dufresne wrote:
Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
The CODA960 manual states that ASO/FMO features of baseline are not supported, so for this reason this driver should only report constrained baseline support.
I know the encoder doesn't support this, but is this also true of the decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder support for both baseline profile and constrained base line profile.
Hmm, double checking, you are right this is documented in the encoding tools sections, not the decoding. But there is extra buffers that need to be passed for ASO/FMO to work, I greatly doubt you have ever tested it.
And you are correct, I don't think I use any test streams that have ASO/FMO enabled.
This is not supported by GStreamer parser, or FFMPEG parsers either. Again, we need to make sure in V2 that encoding and decoding capabilities are well seperated.
As for advertising ASO/FMO, I can leave it there, but be aware I won't be testing it. I can provide you links to streams if you care (they are publicly accessible throught the ITU conformance streams published by the ITU).
That would be welcome.
https://www.itu.int/net/ITU-T/sigdb/spevideo/VideoForm-s.aspx?val=102002641
Notably, if you download the AVCv1 series, there is at least FM2_SVA_C.zip that uses FMO (slice groups). I haven't looked them up, I barely started harnessing it for GStreamer.
You can find the link to everything else here, including HEVC.
https://www.itu.int/net/ITU-T/sigdb/spevideo/Hseries-s.htm
But as for GStreamer and FFMPEG, this is not supported anyway.
Ok, how about changing the commit message to say that this is unsupported for the encoder and untested for the decoder because there is no userspace support?
Agreed.
regards Philipp
Le vendredi 17 juillet 2020 à 10:14 +0200, Philipp Zabel a écrit :
On Fri, 2020-07-17 at 00:49 -0300, Ezequiel Garcia wrote:
From: Nicolas Dufresne nicolas.dufresne@collabora.com
The CODA960 manual states that ASO/FMO features of baseline are not supported, so for this reason this driver should only report constrained baseline support.
I know the encoder doesn't support this, but is this also true of the decoder? The i.MX6DQ Reference Manual explicitly lists H.264/AVC decoder support for both baseline profile and constrained base line profile.
This fixes negotiation issue with constrained baseline content on GStreamer 1.17.1.
Cc: stable@vger.kernel.org Fixes: 42a68012e67c2 ("media: coda: add read-only h.264 decoder profile/level controls") Signed-off-by: Nicolas Dufresne nicolas.dufresne@collabora.com Signed-off-by: Ezequiel Garcia ezequiel@collabora.com
drivers/media/platform/coda/coda-common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 3ab3d976d8ca..c641d1608825 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2335,8 +2335,8 @@ static void coda_encode_ctrls(struct coda_ctx *ctx) V4L2_CID_MPEG_VIDEO_H264_CHROMA_QP_INDEX_OFFSET, -12, 12, 1, 0); v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE,
V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, 0x0,V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE);
V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE, 0x0,V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE);Encoder support is listed as baseline, not constrained baseline, in the manual, but the SPS NALs produced by the encoder start with: 00 00 00 01 67 42 40 ^ so that is profile_idc=66, constraint_set1_flag==1, constrained baseline indeed. I think this change is correct.
if (ctx->dev->devtype->product == CODA_HX4 || ctx->dev->devtype->product == CODA_7541) { v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, @@ -2417,7 +2417,7 @@ static void coda_decode_ctrls(struct coda_ctx *ctx) ctx->h264_profile_ctrl = v4l2_ctrl_new_std_menu(&ctx->ctrls, &coda_ctrl_ops, V4L2_CID_MPEG_VIDEO_H264_PROFILE, V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
~((1 << V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE) |I'm not sure about this one.
Indeed, the decoder does support ASO/FMO, assuming the extra buffer space is allocated as per the documentation (says that slice_save_size should be the max_width * max_height * 3 / 4). Did you have that implemented ? If not, I'd keep that patch, but add a comment to explain that it can be enabled later.
regards Philipp
linux-stable-mirror@lists.linaro.org
-
Ezequiel Garcia -
Nicolas Dufresne -
Philipp Zabel