| Message ID | 1500615659-5332-1-git-send-email-zhong.li@intel.com |
|---|---|
| State | New |
| Headers | show |
2017-07-21 13:40 GMT+08:00 Zhong Li <zhong.li@intel.com>: > Signed-off-by: Zhong Li <zhong.li@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; > 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")) { you can use av_stristr > + 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); > + 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; > } > -- > 1.8.3.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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). > Signed-off-by: Zhong Li <zhong.li@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. > + 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. > + 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,
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Nicolas George > Sent: Friday, July 21, 2017 3:52 PM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Cc: Li, Zhong <zhong.li@intel.com>; sw@jkqxz.net; Zhao, Jun > <jun.zhao@intel.com>; nfxjfg@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@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
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Steven Liu > Sent: Friday, July 21, 2017 3:18 PM > To: FFmpeg development discussions and patches > <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH] doc/examples/encode_video: add qsv > encoder support > > 2017-07-21 13:40 GMT+08:00 Zhong Li <zhong.li@intel.com>: > > Signed-off-by: Zhong Li <zhong.li@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; > > 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")) { > you can use av_stristr Sorry for late reply. av_stristr is case-insensitive but it is expected case-sensitive here. > > + 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); > > + 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; > > } > > -- > > 1.8.3.1 > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
L'octidi 8 thermidor, an CCXXV, Li, Zhong a écrit : > It is similar to hw_device_match_type_in_name() in ffmpeg_hw.c: > if (strstr(codec_name, type_name)) > return type; I must say, I really do not like the idea of having an API example making that terrible construct official. If testing the hw device type is really absolutely necessary in the API, then we need some kind of avcodec_get_hw_type() function to do that cleanly, before considering the API usable and documenting it. Testing the codec name in external code is a big no, testing part of it for a three-letter substring even more so. Regards,
On 02/08/17 07:28, Nicolas George wrote: > L'octidi 8 thermidor, an CCXXV, Li, Zhong a écrit : >> It is similar to hw_device_match_type_in_name() in ffmpeg_hw.c: >> if (strstr(codec_name, type_name)) >> return type; > > I must say, I really do not like the idea of having an API example > making that terrible construct official. > > If testing the hw device type is really absolutely necessary in the API, > then we need some kind of avcodec_get_hw_type() function to do that > cleanly, before considering the API usable and documenting it. > > Testing the codec name in external code is a big no, testing part of it > for a three-letter substring even more so. Yes. The current API behaviour all assumes that the user knows what sort of device they want to supply for a given named codec. The use in ffmpeg is a temporary hack to avoid breaking existing devices while we decide how to express this properly in avcodec. A full solution would be something like adding a new public field to AVCodec with an array of possible device types and corresponding hardware pixfmts. The user can then supply a device of one of those types as AVCodecContext.hw_device_ctx, and see the matching pixfmt later in get_format() if it is supported. The qsv codecs would have AV_HWDEVICE_TYPE_QSV / AV_PIX_FMT_QSV in this field, hwaccelerable codecs could have multiple depending on the build configuration. (This is also solving the related problem that there isn't currently any way to know the matching pixfmt for a device type on a codec, hence for example the table in the hw_decode example.) I'll get around to finishing this off at some point in the not-too-distant future. - Mark
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; 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")) { + 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); + 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; }
Signed-off-by: Zhong Li <zhong.li@intel.com> --- doc/examples/encode_video.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)