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

James Almer jamrial at gmail.com
Sat Nov 9 23:47:02 EET 2019


On 11/9/2019 6:15 PM, Derek Buitenhuis wrote:
> 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().

No, this encoder doesn't have an AVCodec->encode2() implementation, so
it can't be used with the avcodec_encode_video2() API, only with the
avcodec_send_frame()/avcodec_receive_packet() one, so no need to take
user provided packets into consideration since those are not an option.
If you use ff_alloc_packet2(), you'll be first copying the RaPacket to
some internal buffer, which will then be copied into a ref counted
buffer before being returned to the user.

You can safely use av_new_packet() to allocate the packet buffer, as the
AVPacket passed to AVCodec->receive_packet() will be freshly initialized
and empty.

> 
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 



More information about the ffmpeg-devel mailing list