[FFmpeg-devel] [PATCHv4] avcodec: Add librav1e encoder

[James Almer]

On 7/10/2019 9:22 AM, Derek Buitenhuis wrote:
> On 09/07/2019 22:06, James Almer wrote:
>>> @@ -3174,6 +3176,7 @@ libopenmpt_demuxer_deps="libopenmpt"
>>>  libopus_decoder_deps="libopus"
>>>  libopus_encoder_deps="libopus"
>>>  libopus_encoder_select="audio_frame_queue"
>>> +librav1e_encoder_deps="librav1e"
>>
>> Needs to enable extract_extradata_bsf with a _select line as well.
> 
> Fixed locally.
> 
>>> +    if (ctx->tiles >= 0) {
>>> +        rret = rav1e_config_parse_int(cfg, "tiles", ctx->speed);
>>
>> ctx->tiles
> 
> Fixed locally.
> 
>>
>> Also, it may be a good idea to instead look into making the "tiles"
>> option parsing code in libaomenc shared and reuse it here, so it's
>> consistent between encoders. With it you can pass a Cols x Rows string
>> using the AV_OPT_TYPE_IMAGE_SIZE AVOption type and derive values you can
>> then map to rav1e's tile_rows_log2 and tile_cols_log2 options.
> 
> Heavily disagree. rav1e has its own 'tiles' option that determines the right
> cols/rows to use for a reason, and I will not be emulating libaomenc's algo
> instead of using the one provided by rav1e. It's confusing at best (since
> it goes against the rav1e API, and makes the official CLI and ffmpeg work
> differently).

It's not libaom code, it's Mark Thompson's code. It derives
tile_rows_log2 and tile_cols_log2 values, which are what libaom expects,
from a Cols X Rows string (image size AVOption type), so you can tell
the encoder if you want a sample with exactly 4x2 tiles or such.

But if you prefer the tiles option to match the behavior or rav1e's CLI,
then I'm fine with it.

> 
>> In the meantime, you can define tile-columns and tile-rows AVOptions and
>> directly map them to the aforementioned rav1e options.
> 
> Why? What's the use-case? "Being the same as libaomenc" is not a good use case.

Command line option consistency between encoders is a good reason. It's
why you're using "qp" instead of "quantizer" or whatever, after all.
Rav1e lets you set cols and rows in log2 values much like libaom, right?
Then why not add options that map directly to them? Even if you don't
use Mark's code to convert Cols x Rows strings, both a tiles and
tile-cols/rows options can coexist, with the latter two taking
precedence over the former, since they give the user more control over
tiling.

> 
>>> +    case RA_ENCODER_STATUS_ENCODED:
>>> +        if (avctx->internal->draining)
>>> +            goto retry;
>>> +        return AVERROR(EAGAIN);
>>
>> Is rav1e_send_frame() guaranteed to not return
>> RA_ENCODER_STATUS_ENOUGH_DATA if called right after
>> rav1e_receive_packet() returned RA_ENCODER_STATUS_ENCODED?
>> In other words, if it encoded a frame but didn't return it, is it safe
>> to assume its buffers are not full and will not reject new data passed
>> to it?
> 
> rav1e, in fact, never returns this error. I need to check what the
> guarantee is, though - I'm not sure what the future plans are.

Ok, should be good then.

> 
>> If not, then we would have to do goto retry here unconditionally,
>> draining or not. The doxy for avcodec_receive_packet and
>> avcodec_send_frame state that when one returns EAGAIN, the other is
>> guaranteed to return something else.
> 
> [...]
> 
> - 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".
>