[FFmpeg-devel] avdevice/v4l2enc: Allow writing non-rawvideos to v4l2

Resubmit of a previous patch, not sure why the diff didn't come through.

On 26/04/2020 21:26, David Manouchehri wrote:
> Resubmit of a previous patch, not sure why the diff didn't come through.
> 
> From d125fea410dea1c2d4bd791a7472a72822de54a3 Mon Sep 17 00:00:00 2001
> From: David Manouchehri <david.manouchehri@riseup.net>
> Date: Sat, 4 Nov 2017 16:32:41 -0400
> Subject: [PATCH] avdevice/v4l2enc: Allow writing non-rawvideos to v4l2.
> 
> Signed-off-by: David Manouchehri <david.manouchehri@riseup.net>
> ---
>  libavdevice/v4l2enc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/libavdevice/v4l2enc.c b/libavdevice/v4l2enc.c
> index 1c36f81f90..f89ff50dbb 100644
> --- a/libavdevice/v4l2enc.c
> +++ b/libavdevice/v4l2enc.c
> @@ -47,8 +47,7 @@ static av_cold int write_header(AVFormatContext *s1)
>      }
>  
>      if (s1->nb_streams != 1 ||
> -        s1->streams[0]->codecpar->codec_type != AVMEDIA_TYPE_VIDEO ||
> -        s1->streams[0]->codecpar->codec_id   != AV_CODEC_ID_RAWVIDEO) {
> +        s1->streams[0]->codecpar->codec_type != AVMEDIA_TYPE_VIDEO) {
>          av_log(s1, AV_LOG_ERROR,
>                 "V4L2 output device supports only a single raw video stream\n");
>          return AVERROR(EINVAL);
> @@ -56,7 +55,13 @@ static av_cold int write_header(AVFormatContext *s1)
>  
>      par = s1->streams[0]->codecpar;
>  
> -    v4l2_pixfmt = ff_fmt_ff2v4l(par->format, AV_CODEC_ID_RAWVIDEO);
> +    if(s1->streams[0]->codecpar->codec_id == AV_CODEC_ID_RAWVIDEO) {
> +        v4l2_pixfmt = ff_fmt_ff2v4l(par->format, AV_CODEC_ID_RAWVIDEO);
> +    }
> +    else {
> +        v4l2_pixfmt = ff_fmt_ff2v4l(AV_PIX_FMT_NONE, s1->streams[0]->codecpar->codec_id);
> +    }
> +
>      if (!v4l2_pixfmt) { // XXX: try to force them one by one?
>          av_log(s1, AV_LOG_ERROR, "Unknown V4L2 pixel format equivalent for %s\n",
>                 av_get_pix_fmt_name(par->format));
> -- 
> 2.17.1

LGTM.  If noone else has any comments I'll apply this in two days.

Thanks,

- Mark

On Sun, Apr 26, 2020 at 11:26 PM David Manouchehri
<david.manouchehri@riseup.net> wrote:
>
> Resubmit of a previous patch, not sure why the diff didn't come through.
> _______________________________________________
>
> @@ -56,7 +55,13 @@  static av_cold int write_header(AVFormatContext *s1)
>
>      par = s1->streams[0]->codecpar;
>
> -    v4l2_pixfmt = ff_fmt_ff2v4l(par->format, AV_CODEC_ID_RAWVIDEO);
> +    if(s1->streams[0]->codecpar->codec_id == AV_CODEC_ID_RAWVIDEO) {
> +        v4l2_pixfmt = ff_fmt_ff2v4l(par->format, AV_CODEC_ID_RAWVIDEO);
> +    }
> +    else {
> +        v4l2_pixfmt = ff_fmt_ff2v4l(AV_PIX_FMT_NONE, s1->streams[0]->codecpar->codec_id);
> +    }
> +

Hi,

A small nit. Wouldn't the variable `par` be usable there that was just
created right on top of this if/else structure ? A la `par->codec_id`
instead of poking at s1->streams[0] again?

Otherwise looks good to me, linked this today on IRC as someone needed
to test a v4l2 device they were developing with MJPEG. Didn't work,
but that could be due to our MJPEG encoder adding all the metadata
headers etc into the bit stream.

Best regards,
Jan

On 04/05/2020 15:19, Jan Ekström wrote:
> On Sun, Apr 26, 2020 at 11:26 PM David Manouchehri
> <david.manouchehri@riseup.net> wrote:
>>
>> Resubmit of a previous patch, not sure why the diff didn't come through.
>> _______________________________________________
>>
>> @@ -56,7 +55,13 @@  static av_cold int write_header(AVFormatContext *s1)
>>
>>      par = s1->streams[0]->codecpar;
>>
>> -    v4l2_pixfmt = ff_fmt_ff2v4l(par->format, AV_CODEC_ID_RAWVIDEO);
>> +    if(s1->streams[0]->codecpar->codec_id == AV_CODEC_ID_RAWVIDEO) {
>> +        v4l2_pixfmt = ff_fmt_ff2v4l(par->format, AV_CODEC_ID_RAWVIDEO);
>> +    }
>> +    else {
>> +        v4l2_pixfmt = ff_fmt_ff2v4l(AV_PIX_FMT_NONE, s1->streams[0]->codecpar->codec_id);
>> +    }
>> +
> 
> Hi,
> 
> A small nit. Wouldn't the variable `par` be usable there that was just
> created right on top of this if/else structure ? A la `par->codec_id`
> instead of poking at s1->streams[0] again?
> 
> Otherwise looks good to me, linked this today on IRC as someone needed
> to test a v4l2 device they were developing with MJPEG. Didn't work,
> but that could be due to our MJPEG encoder adding all the metadata
> headers etc into the bit stream.

Yep, applied with that change.

Thank you!

- Mark