| Message ID | 20200717034923.219524-2-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] media: coda: Fix reported H264 profile | expand |
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. > > [0] https://www.chipsnmedia.com/fullhd 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
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,