[vlc-devel] [PATCH 3/3] x264: the frame rate of the output block is one of the output format

[Steve Lhomme]

Yes, there's a lot more to clean in x264 or encoders in general. For 
example they write in fmt_in even if they return an error. The next 
encoder in line will receive these changes on fmt_in.

Just like the display modules they should be given a read-only format 
(or make sure they don't write on it) and give the modified format on 
output of the Open(). fmt_out should probably using this as well.

The rationale for this patch is that data coming out of encoders should 
follow the fmt_out format, not the fmt_in. If the fmt_out is bogus or 
not set that is a different problem. But luckily in this case it's correct.

On 2021-01-15 10:15, Alexandre Janniaux wrote:
> Hi,
> 
> Sorry misread the first snippet of code, then first and
> the second snippet should probably be fixed to only use
> fmt_out. ;)
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Fri, Jan 15, 2021 at 10:13:33AM +0100, Alexandre Janniaux wrote:
>> Hi,
>>
>> This seems logical but are you sure this is correct as is?
>>
>> The code in the transcode seems to assign the framerate to
>> the input video format and not the output video format:
>>
>>
>>      transcode_video_framerate_apply( p_src, p_enc_out );
>>      p_enc_in->i_frame_rate = p_enc_out->i_frame_rate;
>>      p_enc_in->i_frame_rate_base = p_enc_out->i_frame_rate_base;
>>      msg_Dbg( p_obj, "source fps %u/%u, destination %u/%u",
>>               p_dec_out->i_frame_rate, p_dec_out->i_frame_rate_base,
>>               p_enc_in->i_frame_rate, p_enc_in->i_frame_rate_base );
>>
>> And in x264, it's used there from fmt_in:
>>
>>      if( p_enc->fmt_in.video.i_frame_rate_base > 0 )
>>      {
>>          p_sys->param.i_fps_num = p_enc->fmt_in.video.i_frame_rate;
>>          p_sys->param.i_fps_den = p_enc->fmt_in.video.i_frame_rate_base;
>>          p_sys->param.b_vfr_input = 0;
>>      }
>>
>> There's not much rationale in the commit message that would
>> help understand why this should be in the output format and
>> why it was wrong before.
>>
>> Regards,
>> --
>> Alexandre Janniaux
>> Videolabs
>>
>> On Tue, Jan 12, 2021 at 05:41:26PM +0100, Steve Lhomme wrote:
>>> ---
>>>   modules/codec/x264.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/modules/codec/x264.c b/modules/codec/x264.c
>>> index dbb8ea2ddc7..70049f50b8b 100644
>>> --- a/modules/codec/x264.c
>>> +++ b/modules/codec/x264.c
>>> @@ -1546,8 +1546,8 @@ static block_t *Encode( encoder_t *p_enc, picture_t *p_pict )
>>>       {
>>>           /* This isn't really valid for streams with B-frames */
>>>           p_block->i_length = vlc_tick_from_samples(
>>> -                    p_enc->fmt_in.video.i_frame_rate_base,
>>> -                    p_enc->fmt_in.video.i_frame_rate );
>>> +                    p_enc->fmt_out.video.i_frame_rate_base,
>>> +                    p_enc->fmt_out.video.i_frame_rate );
>>>       }
>>>
>>>       /* scale pts-values back*/
>>> --
>>> 2.29.2
>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>