[FFmpeg-devel] [PATCH v5] avcodec: Add librav1e encoder

[Derek Buitenhuis]

On 09/11/2019 18:03, James Almer wrote:
>> +    if (ctx->tile_rows >= 0) {
> 
> Since these are no longer log2 values, does rav1e change 0 to 1 internally?
> It may be a better idea to make 0 the default, and only call
> rav1e_config_parse_int() if it's > 0.

Yes.

Changed to match this.

>> +    if (ctx->tile_cols >= 0) {
> 
> Ditto.

Fixed.

> 
>> +        rret = rav1e_config_parse_int(cfg, "tile_cols_log2", ctx->tile_cols);
> 
> Should be "tile_cols".

Fixed.

>> +        rret = rav1e_config_parse_int(cfg, "bitrate", avctx->bit_rate);
>> +        if (rret < 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Could not set bitrate.\n");
>> +            ret = AVERROR_INVALIDDATA;
>> +            goto end;
>> +        }
>> +    } else if (ctx->quantizer >= 0) {
> 
> Bitrate will be ignored if set. Maybe the doxy could mention it, or a
> log message printed here to let the user know about it.

I've added a warning if both are set.

>> +    switch (ret) {
>> +    case RA_ENCODER_STATUS_SUCCESS:
>> +        break;
>> +    case RA_ENCODER_STATUS_ENOUGH_DATA:
>> +        return AVERROR(EAGAIN);
>> +    case RA_ENCODER_STATUS_FAILURE:
>> +        av_log(avctx, AV_LOG_ERROR, "Could not send frame.\n");
>> +        return AVERROR_EXTERNAL;
>> +    default:
>> +        av_log(avctx, AV_LOG_ERROR, "Unknown return code %d from rav1e_send_frame.\n", ret);
>> +        return AVERROR_UNKNOWN;
>> +    }
> 
> You could use rav1e_status_to_str() to get the error string and print it
> for the STATUS_FAILURE and default cases.

Done, but I've kept it inside the switch.

> Ditto here. Only the custom NEED_MORE_DATA message printed while in
> draining mode is worth keeping.

Done.

>> +
>> +    pkt->buf = av_buffer_create((uint8_t *) rpkt->data, rpkt->len, librav1e_packet_unref,
>> +                                rpkt, AV_BUFFER_FLAG_READONLY);
> 
> When i came up with this zero-copy method i didn't realize that rav1e
> may not be padding the buffer in question. If the padding is not at
> least AV_INPUT_BUFFER_PADDING_SIZE big, then it's technically breaking
> the AVPacket API, and we may have to use av_new_packet() and copy the
> buffer instead.

I don't think we can guarantee AV_INPUT_BUFFER_PADDING_SIZE in rav1e's
API for packet data.

I also assume you meant ff_alloc_packet2(), and not av_new_packet().

I've converted it to that.

>> +    { "tile-rows", "number of tiles rows to encode with", OFFSET(tile_rows), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
>> +    { "tile-columns", "number of tiles columns to encode with", OFFSET(tile_cols), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
> 
> These two are not documented.

Fixed.

Thanks for the review! New patch sent.

- Derek