Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Luca Barbato]

On 12/03/2018 14:00, Diego Biurrun wrote:

On Mon, Mar 12, 2018 at 12:11:22PM +0100, Luca Barbato wrote:

--- /dev/null
+++ b/libavcodec/libaomenc.c
@@ -0,0 +1,583 @@
+// provide dummy value to initialize wrapper, values will be updated each 
_encode()
+aom_img_wrap(>rawimg, ff_aom_pixfmt_to_imgfmt(avctx->pix_fmt),
+ avctx->width, avctx->height, 1, (unsigned char *)1);


What's with the strange cast?


What the comment says, a dummy value that gets updated with the correct 
values once the frame is available.


You might notice that the same code is in libvpxenc.

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Diego Biurrun]

On Mon, Mar 12, 2018 at 12:11:22PM +0100, Luca Barbato wrote:
> --- /dev/null
> +++ b/libavcodec/libaomenc.c
> @@ -0,0 +1,583 @@
> +// provide dummy value to initialize wrapper, values will be updated 
> each _encode()
> +aom_img_wrap(>rawimg, ff_aom_pixfmt_to_imgfmt(avctx->pix_fmt),
> + avctx->width, avctx->height, 1, (unsigned char *)1);

What's with the strange cast?


Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Luca Barbato]

On 27/02/2018 16:13, Vittorio Giovara wrote:

On Tue, Feb 27, 2018 at 5:25 AM, Luca Barbato  wrote:


On 26/02/2018 18:21, Vittorio Giovara wrote:


On Mon, Feb 26, 2018 at 12:10 AM, Luca Barbato 
wrote:

---


+
+if (avctx->qmin > 0)
+enccfg.rc_min_quantizer = avctx->qmin;
+if (avctx->qmax > 0)
+enccfg.rc_max_quantizer = avctx->qmax;
+
+#if FF_API_PRIVATE_OPT
+FF_DISABLE_DEPRECATION_WARNINGS
+if (avctx->frame_skip_threshold)
+ctx->drop_threshold = avctx->frame_skip_threshold;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+enccfg.rc_dropframe_thresh = ctx->drop_threshold;
+




+static int storeframe(AVCodecContext *avctx, struct FrameListData

*cx_frame,
+  AVPacket *pkt)
+{
+int ret = ff_alloc_packet(pkt, cx_frame->sz);
+if (ret >= 0) {
+memcpy(pkt->data, cx_frame->buf, pkt->size);
+pkt->pts = pkt->dts = cx_frame->pts;
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx->coded_frame->pts   = cx_frame->pts;
+avctx->coded_frame->key_frame = !!(cx_frame->flags &
AOM_FRAME_IS_KEY);
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+pkt->flags |= AV_PKT_FLAG_KEY;
+} else {
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+}
+} else {



I don't think we should add deprecated code.



We have to if we want to keep compatibility.



No, compatibility is not needed for new APIs and new codecs.
If you want to implement this functionality, you should add the proper side
data, see 40cf1bbacc6220a0aa6bed5c331871d43f9ce370.



Then I'd drop that and have less code to care about :)

lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Vittorio Giovara]

On Tue, Feb 27, 2018 at 5:25 AM, Luca Barbato  wrote:

> On 26/02/2018 18:21, Vittorio Giovara wrote:
>
>> On Mon, Feb 26, 2018 at 12:10 AM, Luca Barbato 
>> wrote:
>>
>> ---
>>>
>>> +
>>> +if (avctx->qmin > 0)
>>> +enccfg.rc_min_quantizer = avctx->qmin;
>>> +if (avctx->qmax > 0)
>>> +enccfg.rc_max_quantizer = avctx->qmax;
>>> +
>>> +#if FF_API_PRIVATE_OPT
>>> +FF_DISABLE_DEPRECATION_WARNINGS
>>> +if (avctx->frame_skip_threshold)
>>> +ctx->drop_threshold = avctx->frame_skip_threshold;
>>> +FF_ENABLE_DEPRECATION_WARNINGS
>>> +#endif
>>> +enccfg.rc_dropframe_thresh = ctx->drop_threshold;
>>> +
>>>
>>>
>>
>> +static int storeframe(AVCodecContext *avctx, struct FrameListData
>>> *cx_frame,
>>> +  AVPacket *pkt)
>>> +{
>>> +int ret = ff_alloc_packet(pkt, cx_frame->sz);
>>> +if (ret >= 0) {
>>> +memcpy(pkt->data, cx_frame->buf, pkt->size);
>>> +pkt->pts = pkt->dts = cx_frame->pts;
>>> +#if FF_API_CODED_FRAME
>>> +FF_DISABLE_DEPRECATION_WARNINGS
>>> +avctx->coded_frame->pts   = cx_frame->pts;
>>> +avctx->coded_frame->key_frame = !!(cx_frame->flags &
>>> AOM_FRAME_IS_KEY);
>>> +FF_ENABLE_DEPRECATION_WARNINGS
>>> +#endif
>>> +
>>> +if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
>>> +#if FF_API_CODED_FRAME
>>> +FF_DISABLE_DEPRECATION_WARNINGS
>>> +avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
>>> +FF_ENABLE_DEPRECATION_WARNINGS
>>> +#endif
>>> +pkt->flags |= AV_PKT_FLAG_KEY;
>>> +} else {
>>> +#if FF_API_CODED_FRAME
>>> +FF_DISABLE_DEPRECATION_WARNINGS
>>> +avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;
>>> +FF_ENABLE_DEPRECATION_WARNINGS
>>> +#endif
>>> +}
>>> +} else {
>>>
>>>
>> I don't think we should add deprecated code.
>>
>>
> We have to if we want to keep compatibility.
>

No, compatibility is not needed for new APIs and new codecs.
If you want to implement this functionality, you should add the proper side
data, see 40cf1bbacc6220a0aa6bed5c331871d43f9ce370.
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Luca Barbato]

On 26/02/2018 18:21, Vittorio Giovara wrote:

On Mon, Feb 26, 2018 at 12:10 AM, Luca Barbato  wrote:


---

+
+if (avctx->qmin > 0)
+enccfg.rc_min_quantizer = avctx->qmin;
+if (avctx->qmax > 0)
+enccfg.rc_max_quantizer = avctx->qmax;
+
+#if FF_API_PRIVATE_OPT
+FF_DISABLE_DEPRECATION_WARNINGS
+if (avctx->frame_skip_threshold)
+ctx->drop_threshold = avctx->frame_skip_threshold;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+enccfg.rc_dropframe_thresh = ctx->drop_threshold;
+





+static int storeframe(AVCodecContext *avctx, struct FrameListData
*cx_frame,
+  AVPacket *pkt)
+{
+int ret = ff_alloc_packet(pkt, cx_frame->sz);
+if (ret >= 0) {
+memcpy(pkt->data, cx_frame->buf, pkt->size);
+pkt->pts = pkt->dts = cx_frame->pts;
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx->coded_frame->pts   = cx_frame->pts;
+avctx->coded_frame->key_frame = !!(cx_frame->flags &
AOM_FRAME_IS_KEY);
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
+if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+pkt->flags |= AV_PKT_FLAG_KEY;
+} else {
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+}
+} else {



I don't think we should add deprecated code.



We have to if we want to keep compatibility.

lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Vittorio Giovara]

On Mon, Feb 26, 2018 at 12:10 AM, Luca Barbato  wrote:

> ---
>
> +
> +if (avctx->qmin > 0)
> +enccfg.rc_min_quantizer = avctx->qmin;
> +if (avctx->qmax > 0)
> +enccfg.rc_max_quantizer = avctx->qmax;
> +
> +#if FF_API_PRIVATE_OPT
> +FF_DISABLE_DEPRECATION_WARNINGS
> +if (avctx->frame_skip_threshold)
> +ctx->drop_threshold = avctx->frame_skip_threshold;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +enccfg.rc_dropframe_thresh = ctx->drop_threshold;
> +
>


> +static int storeframe(AVCodecContext *avctx, struct FrameListData
> *cx_frame,
> +  AVPacket *pkt)
> +{
> +int ret = ff_alloc_packet(pkt, cx_frame->sz);
> +if (ret >= 0) {
> +memcpy(pkt->data, cx_frame->buf, pkt->size);
> +pkt->pts = pkt->dts = cx_frame->pts;
> +#if FF_API_CODED_FRAME
> +FF_DISABLE_DEPRECATION_WARNINGS
> +avctx->coded_frame->pts   = cx_frame->pts;
> +avctx->coded_frame->key_frame = !!(cx_frame->flags &
> AOM_FRAME_IS_KEY);
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +
> +if (!!(cx_frame->flags & AOM_FRAME_IS_KEY)) {
> +#if FF_API_CODED_FRAME
> +FF_DISABLE_DEPRECATION_WARNINGS
> +avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +pkt->flags |= AV_PKT_FLAG_KEY;
> +} else {
> +#if FF_API_CODED_FRAME
> +FF_DISABLE_DEPRECATION_WARNINGS
> +avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;
> +FF_ENABLE_DEPRECATION_WARNINGS
> +#endif
> +}
> +} else {
>

I don't think we should add deprecated code.
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Jean-Baptiste Kempf]

Hello,

On Mon, 26 Feb 2018, at 14:00, Diego Biurrun wrote:
> > Since the bitstream will be frozen soon shouldn't hurt adding it.
> 
> I'd prefer if you waited until it is actually frozen.

I kind of disagree: It will take time to review.
So, it is better to merge when patch is clean, with EXPERIMENTAL CAP, and 
remove it when ready.

Best,

-- 
Jean-Baptiste Kempf -  President
+33 672 704 734
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[James Almer]

On 2/26/2018 2:10 AM, Luca Barbato wrote:
> ---
> 
> Since the bitstream will be frozen soon shouldn't hurt adding it.
> 
>  configure  |   1 +
>  libavcodec/Makefile|   1 +
>  libavcodec/allcodecs.c |   2 +-
>  libavcodec/libaomenc.c | 602 
> +
>  4 files changed, 605 insertions(+), 1 deletion(-)
>  create mode 100644 libavcodec/libaomenc.c
> 

[...]

> +/* 0-3: For non-zero values the encoder increasingly optimizes for 
> reduced
> +   complexity playback on low powered devices at the expense of encode
> +   quality. */
> +if (avctx->profile != FF_PROFILE_UNKNOWN)
> +enccfg.g_profile = avctx->profile;
> +else if (avctx->pix_fmt == AV_PIX_FMT_YUV420P)
> +avctx->profile = enccfg.g_profile = FF_PROFILE_VP9_0;
> +else
> +avctx->profile = enccfg.g_profile = FF_PROFILE_VP9_1;

You should add profiles for AV1 instead.

[...]

> +AVCodec ff_libaom_av1_encoder = {
> +.name   = "libaom-av1",
> +.long_name  = NULL_IF_CONFIG_SMALL("libaom AV1"),
> +.type   = AVMEDIA_TYPE_VIDEO,
> +.id = AV_CODEC_ID_AV1,
> +.priv_data_size = sizeof(AOMContext),
> +.init   = aom_init,
> +.encode2= aom_encode,
> +.close  = aom_free,
> +.capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,

AV_CODEC_CAP_EXPERIMENTAL. libvpx_vp9 used to outright forbid encoding
with some libvpx versions *after* the bitstream was frozen, let alone
before, since it was known to produce bad streams in some situations. So
at the very least don't just let users encode potentially bad AV1 files
unknowingly.

This can be changed once a stable/good version is released.

> +.pix_fmts   = (const enum AVPixelFormat[]){ AV_PIX_FMT_YUV420P, 
> AV_PIX_FMT_NONE },
> +.priv_class = _aom,
> +.defaults   = defaults,
> +.wrapper_name   = "libaom",
> +};
> --
> 2.12.2
> 
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
> 

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Luca Barbato]

On 26/02/2018 14:00, Diego Biurrun wrote:

On Mon, Feb 26, 2018 at 06:10:42AM +0100, Luca Barbato wrote:

---

Since the bitstream will be frozen soon shouldn't hurt adding it.


I'd prefer if you waited until it is actually frozen.



Given the amount of nits you found (and that should be addressed in the 
vpx wrapper as well), probably the timing is good.


lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] Support AV1 encoding using libaom

[Diego Biurrun]

On Mon, Feb 26, 2018 at 06:10:42AM +0100, Luca Barbato wrote:
> ---
> 
> Since the bitstream will be frozen soon shouldn't hurt adding it.

I'd prefer if you waited until it is actually frozen.

>  configure  |   1 +
>  libavcodec/Makefile|   1 +
>  libavcodec/allcodecs.c |   2 +-
>  libavcodec/libaomenc.c | 602 
> +
>  4 files changed, 605 insertions(+), 1 deletion(-)

docs, changelog

> --- /dev/null
> +++ b/libavcodec/libaomenc.c
> @@ -0,0 +1,602 @@
> +
> +#include "avcodec.h"
> +#include "internal.h"
> +#include "libaom.h"
> +#include "libavutil/base64.h"
> +#include "libavutil/common.h"
> +#include "libavutil/mathematics.h"
> +#include "libavutil/opt.h"

canonical header ordering please

> +static av_cold void dump_enc_cfg(AVCodecContext *avctx,
> + const struct aom_codec_enc_cfg *cfg)
> +{
> +int width = -30;
> +int level = AV_LOG_DEBUG;
> +
> +av_log(avctx, level, "aom_codec_enc_cfg\n");
> +av_log(avctx, level, "generic settings\n"
> +   "  %*s%u\n  %*s%u\n  %*s%u\n  %*s%u\n  %*s%u\n"
> +   "  %*s{%u/%u}\n  %*s%u\n  %*s%d\n  %*s%u\n",
> +   width, "g_usage:",   cfg->g_usage,

Looks like pointless indirection to me.

> +static av_cold int codecctl_int(AVCodecContext *avctx,
> +enum aome_enc_control_id id, int val)
> +{
> +AOMContext *ctx = avctx->priv_data;
> +char buf[80];
> +int width = -30;
> +int res;
> +
> +snprintf(buf, sizeof(buf), "%s:", ctlidstr[id]);
> +av_log(avctx, AV_LOG_DEBUG, "  %*s%d\n", width, buf, val);

The format string does not match the number of arguments.

> +res = aom_codec_control(>encoder, id, val);
> +if (res != AOM_CODEC_OK) {
> +snprintf(buf, sizeof(buf), "Failed to set %s codec control",
> + ctlidstr[id]);
> +log_encoder_error(avctx, buf);
> +}
> +
> +return res == AOM_CODEC_OK ? 0 : AVERROR(EINVAL);

I think it's clearer to return from the if above instead of using the
ternary operator here.

> +static av_cold int aom_init(AVCodecContext *avctx)
> +{
> +else
> +enccfg.rc_target_bitrate = av_rescale_rnd(avctx->bit_rate, 1, 1000,
> +  AV_ROUND_NEAR_INF);

Indentation is off.

> +//0-100 (0 => CBR, 100 => VBR)
> +//_enc_init() will balk if kf_min_dist differs from max w/AOM_KF_AUTO

nit: space after //

> +ctx->twopass_stats.sz  = strlen(avctx->stats_in) * 3 / 4;

nit: stray double space before =

> +//codec control failures are currently treated only as warnings
> +//provide dummy value to initialize wrapper, values will be updated each 
> _encode()

nit: space after //

> + * Store coded frame information in format suitable for return from 
> encode2().

in a format

> +static int queue_frames(AVCodecContext *avctx, AVPacket *pkt_out)
> +{
> +memcpy((uint8_t*)stats->buf + stats->sz,

(uint8_t *)

> +case AOM_CODEC_PSNR_PKT: //FIXME add support for AV_CODEC_FLAG_PSNR
> +case AOM_CODEC_CUSTOM_PKT:
> +//ignore unsupported/unrecognized packet types

nit: space after //

> +static int aom_encode(AVCodecContext *avctx, AVPacket *pkt,
> +  const AVFrame *frame, int *got_packet)
> +{
> +if (!frame && avctx->flags & AV_CODEC_FLAG_PASS1) {
> +unsigned int b64_size = AV_BASE64_SIZE(ctx->twopass_stats.sz);
> +
> +avctx->stats_out = av_malloc(b64_size);
> +if (!avctx->stats_out) {
> +av_log(avctx, AV_LOG_ERROR, "Stat buffer alloc (%d bytes) 
> failed\n",
> +   b64_size);

%u bytes, or better yet use size_t and %zu

> +{ "partitions",  "The frame partitions are independently decodable "
> + "by the bool decoder, meaning that partitions can 
> be decoded even "
> + "though earlier partitions have been lost. Note 
> that intra predicition"

predic_tion

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel