[FFmpeg-devel] [PATCH] doc/examples/encode_video: add qsv encoder support

[Li, Zhong]

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Nicolas George
> Sent: Friday, July 21, 2017 3:52 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
> Cc: Li, Zhong <zhong.li at intel.com>; sw at jkqxz.net; Zhao, Jun
> <jun.zhao at intel.com>; nfxjfg at googlemail.com
> Subject: Re: [FFmpeg-devel] [PATCH] doc/examples/encode_video: add qsv
> encoder support
> 
> Le tridi 3 thermidor, an CCXXV, Zhong Li a écrit :
> > Subject: Re: [FFmpeg-devel] [PATCH] doc/examples/encode_video: add qsv
> > encoder support
> 
> I do not think it is a good idea. Examples are meant to be simple. I think a
> separate example for hwdevice encoding would be a better idea, although it
> has drawbacks of its own (code duplication in the utility parts).

Ok, will rework to a separate qsv encoder example.

> 
> > Signed-off-by: Zhong Li <zhong.li at intel.com>
> > ---
> >  doc/examples/encode_video.c | 32
> +++++++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/examples/encode_video.c
> b/doc/examples/encode_video.c
> > index 8cd1321..9c26f63 100644
> > --- a/doc/examples/encode_video.c
> > +++ b/doc/examples/encode_video.c
> > @@ -35,6 +35,8 @@
> >
> >  #include <libavutil/opt.h>
> >  #include <libavutil/imgutils.h>
> > +#include "libavutil/buffer.h"
> > +#include "libavutil/hwcontext.h"
> >
> >  static void encode(AVCodecContext *enc_ctx, AVFrame *frame,
> AVPacket *pkt,
> >                     FILE *outfile)
> > @@ -75,7 +77,10 @@ int main(int argc, char **argv)
> >      FILE *f;
> >      AVFrame *frame;
> >      AVPacket *pkt;
> 
> > +    AVBufferRef* encode_device = NULL;
> 
> Pointer marks belong with the variable, not the type.
> 
> >      uint8_t endcode[] = { 0, 0, 1, 0xb7 };
> > +    enum AVHWDeviceType hw_device_type =
> AV_HWDEVICE_TYPE_NONE;
> > +    enum AVPixelFormat pixel_format = AV_PIX_FMT_YUV420P;
> >
> >      if (argc <= 2) {
> >          fprintf(stderr, "Usage: %s <output file> <codec name>\n",
> > argv[0]); @@ -86,6 +91,21 @@ int main(int argc, char **argv)
> >
> >      avcodec_register_all();
> >
> 
> > +    if (strstr(codec_name, "qsv")) {
> 
> If this is necessary, then someone seriously messed up the API design.
> Fortunately, I see no trace of it in ffmpeg*.c, so I guess the correct way of
> detecting hwdevice encoding is not that.

It is similar to hw_device_match_type_in_name() in ffmpeg_hw.c:
	if (strstr(codec_name, type_name))
       return type;

> 
> > +        hw_device_type = AV_HWDEVICE_TYPE_QSV;
> > +        pixel_format = AV_PIX_FMT_NV12;
> > +    }
> > +
> > +    /* open the hardware device */
> > +    if (hw_device_type != AV_HWDEVICE_TYPE_NONE) {
> 
> > +        ret = av_hwdevice_ctx_create(&encode_device,
> hw_device_type,
> > +                                     NULL, NULL, 0);
> 
> I see that encode_device is only used later for freeing. Can you explain in a
> comment why it is necessary? Otherwise, user may think it is unnecessary
> and remove it.

The encode_device is passed to av_hwdevice_ctx_create to be written, it is necessary for API requirement.

> 
> > +        if (ret < 0) {
> > +            fprintf(stderr, "Cannot open the hardware device\n");
> > +            exit(1);
> > +        }
> > +    }
> > +
> >      /* find the mpeg1video encoder */
> >      codec = avcodec_find_encoder_by_name(codec_name);
> >      if (!codec) {
> > @@ -120,7 +140,7 @@ int main(int argc, char **argv)
> >       */
> >      c->gop_size = 10;
> >      c->max_b_frames = 1;
> > -    c->pix_fmt = AV_PIX_FMT_YUV420P;
> > +    c->pix_fmt = pixel_format;
> >
> >      if (codec->id == AV_CODEC_ID_H264)
> >          av_opt_set(c->priv_data, "preset", "slow", 0); @@ -173,8
> > +193,13 @@ int main(int argc, char **argv)
> >          /* Cb and Cr */
> >          for (y = 0; y < c->height/2; y++) {
> >              for (x = 0; x < c->width/2; x++) {
> > -                frame->data[1][y * frame->linesize[1] + x] = 128 + y + i
> * 2;
> > -                frame->data[2][y * frame->linesize[2] + x] = 64 + x + i
> * 5;
> > +                if (frame->format == AV_PIX_FMT_YUV420P) {
> > +                    frame->data[1][y * frame->linesize[1] + x] = 128
> + y + i * 2;
> > +                    frame->data[2][y * frame->linesize[2] + x] = 64 +
> x + i * 5;
> > +                } else if (frame->format == AV_PIX_FMT_NV12) {
> > +                    frame->data[1][y * frame->linesize[1] + 2 * x] =
> 128 + y + i * 2;
> > +                    frame->data[1][y * frame->linesize[1] + 2 * x + 1]
> = 64 + x + i * 5;
> > +                }
> >              }
> >          }
> >
> > @@ -194,6 +219,7 @@ int main(int argc, char **argv)
> >      avcodec_free_context(&c);
> >      av_frame_free(&frame);
> >      av_packet_free(&pkt);
> > +    av_buffer_unref(&encode_device);
> >
> >      return 0;
> >  }
> 
> Regards,
> 
> --
>   Nicolas George