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

[James Almer]

On 5/28/2019 4:49 PM, Derek Buitenhuis wrote:
> On 28/05/2019 20:32, James Almer wrote:
>>> +    default:
>>> +        // This should be impossible
>>> +        return (RaChromaSampling) -1;
>>
>> If it's not meant to happen, then it should probably be an av_assert0.
> 
> Yeah, that makes more sense. Will change.
> 
>>> +    int rret;
>>> +    int ret = 0;
>>
>> Why two ret variables?
> 
> Will fix.
> 
>>> +    if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
>>> +         const AVBitStreamFilter *filter = av_bsf_get_by_name("extract_extradata");
>>
>> See
>> https://github.com/dwbuiten/FFmpeg/commit/e146749232928a1cebe637338189938644237894#r33622810
>>
>> You don't need to extract the Sequence Header from encoded frames since
>> rav1e provides a function for this purpose.
>> So does libaom, for that matter, but it's buggy and that's why it's not
>> being used.
> 
> As discussed on IRC, that function does not return the config OBUs at the end of
> the ISOBMFF-style data, so this way is still preferred until it does.
> 
> I'll switch over if/once rav1e's API does return that.
> 
> 
>>> +    memcpy(pkt->data, rpkt->data, rpkt->len);
>>> +
>>> +    if (rpkt->frame_type == RA_FRAME_TYPE_KEY)
>>> +        pkt->flags |= AV_PKT_FLAG_KEY;
>>> +
>>> +    pkt->pts = pkt->dts = rpkt->number * avctx->ticks_per_frame;
>>> +
>>> +    rav1e_packet_unref(rpkt);
>>
>> You can avoid the data copy if you wrap the RaPacket into the
>> AVBufferRef used in the AVPacket, but for this you need to use the
>> send/receive API, given the encode2 one allows the caller to use their
>> own allocated packet.
> 
> As discussed on IRC / noted in the top of this email, I'll see how
> you fare switching to send/recieve. :)
> 
>>> +static const AVOption options[] = {
>>> +    { "quantizer", "use constant quantizer mode", OFFSET(quantizer), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 255, VE },
>>> +    { "max-quantizer", "max quantizer when using bitrate mode", OFFSET(max_quantizer), AV_OPT_TYPE_INT, { .i64 = 255 }, 1, 255, VE },
>>
>> This should be mapped to qmax lavc option instead.
> 
> OK. Is there a matching option for plain old quantizer?
> I couldn't find an obvious one.

I think x26* and vpx/aom call it crf? It's not in option_tables.h in any
case.

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