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
_______________________________________________
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".
