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