[FFmpeg-devel] patch for a new H.264 codec

Hi

Here is the patch.

Please check.

Thanks.

Yufei.

Yufei He (12019-02-25):
> From 7dd17409a22858f3c18d832c301fb904508dea3b Mon Sep 17 00:00:00 2001
> From: yhe <yhe@matrox.com>
> Date: Mon, 25 Feb 2019 14:38:21 -0500
> Subject: [PATCH] M264 codec support

Quick comments.

They are in no way to be taken as any kind of endorsement for the patch.

> 
> ---
>  .vscode/settings.json            |  27 +++

No, thanks.

>  Changelog                        |   1 +
>  configure                        |   1 +
>  doc/general.texi                 |   1 +
>  fftools/ffmpeg.c                 |  14 ++
>  libavcodec/.vscode/settings.json |   5 +
>  libavcodec/Makefile              |   5 +
>  libavcodec/allcodecs.c           |   6 +-
>  libavcodec/avcodec.h             |   3 +
>  libavcodec/codec_desc.c          |   9 +
>  libavcodec/m264dec.c             | 476 +++++++++++++++++++++++++++++++++++++++
>  libavcodec/m264dec.h             |  42 ++++
>  libavcodec/m264enc.c             | 289 ++++++++++++++++++++++++
>  libavcodec/m264enc.h             |  39 ++++
>  libavcodec/profiles.c            |   5 +
>  libavcodec/profiles.h            |   1 +
>  16 files changed, 922 insertions(+), 2 deletions(-)
>  create mode 100644 .vscode/settings.json
>  create mode 100644 libavcodec/.vscode/settings.json
>  create mode 100644 libavcodec/m264dec.c
>  create mode 100644 libavcodec/m264dec.h
>  create mode 100644 libavcodec/m264enc.c
>  create mode 100644 libavcodec/m264enc.h
> 
> diff --git a/.vscode/settings.json b/.vscode/settings.json
> new file mode 100644
> index 0000000..4cdd683
> --- /dev/null
> +++ b/.vscode/settings.json
> @@ -0,0 +1,27 @@
> +{
> +   "files.associations": {
> +      "avcodec.h": "c",
> +      "nvdec.h": "c",
> +      "config.h": "c",
> +      "avassert.h": "c",
> +      "iostream": "cpp",
> +      "profiles.h": "c",
> +      "swscale.h": "c",
> +      "array": "c",
> +      "functional": "c",
> +      "hash_map": "c",
> +      "initializer_list": "c",
> +      "utility": "c",
> +      "*.tcc": "cpp",
> +      "deque": "cpp",
> +      "list": "cpp",
> +      "vector": "cpp",
> +      "system_error": "cpp",
> +      "bitset": "cpp",
> +      "valarray": "cpp",
> +      "adpcm_data.h": "c",
> +      "blockdsp.h": "c",
> +      "channel_layout.h": "c",
> +      "h264chroma.h": "c"
> +   }
> +}
> \ No newline at end of file
> diff --git a/Changelog b/Changelog
> index c90f119..f70368b 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -11,6 +11,7 @@ version <next>:
>  - dhav demuxer
>  - PCM-DVD encoder
>  - GIF parser
> +- M264 encoder
>  
>  
>  version 4.1:
> diff --git a/configure b/configure
> index b062b63..161beee 100755
> --- a/configure
> +++ b/configure
> @@ -2618,6 +2618,7 @@ dds_decoder_select="texturedsp"
>  dirac_decoder_select="dirac_parse dwt golomb videodsp mpegvideoenc"
>  dnxhd_decoder_select="blockdsp idctdsp"
>  dnxhd_encoder_select="blockdsp fdctdsp idctdsp mpegvideoenc pixblockdsp"

> +#m264_encoder_select="blockdsp fdctdsp idctdsp mpegvideoenc pixblockdsp"

Leftover debug.

>  dolby_e_decoder_select="mdct"
>  dvvideo_decoder_select="dvprofile idctdsp"
>  dvvideo_encoder_select="dvprofile fdctdsp me_cmp pixblockdsp"
> diff --git a/doc/general.texi b/doc/general.texi
> index 2bc33d1..e012bad 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -828,6 +828,7 @@ following image formats are supported:
>  @item Deluxe Paint Animation @tab     @tab  X
>  @item DNxHD                  @tab   X @tab  X
>      @tab aka SMPTE VC3

> +@item M264                   @tab   X @tab  

Does not belong there.

>  @item Duck TrueMotion 1.0   @tab     @tab  X
>      @tab fourcc: DUCK
>  @item Duck TrueMotion 2.0    @tab     @tab  X
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 6abcc57..b8fa266 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2418,6 +2418,20 @@ static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output, int64_
>      if (!*got_output || ret < 0)
>          return ret;
>  

> +   if(*got_output)        
> +   {
> +      if(decoded_frame->width == 0)
> +      {
> +         av_log(NULL, AV_LOG_DEBUG, "Frame parameters mismatch context %d,%d,%d != %d,%d,%d\n",
> +               decoded_frame->width,
> +               decoded_frame->height,
> +               decoded_frame->format,
> +               ist->dec_ctx->width,
> +               ist->dec_ctx->height,
> +               ist->dec_ctx->pix_fmt);
> +      }
> +   }
> +

Unrelated. And coding style inconsistent.

>      if(ist->top_field_first>=0)
>          decoded_frame->top_field_first = ist->top_field_first;
>  
> diff --git a/libavcodec/.vscode/settings.json b/libavcodec/.vscode/settings.json
> new file mode 100644
> index 0000000..dc1852f
> --- /dev/null
> +++ b/libavcodec/.vscode/settings.json
> @@ -0,0 +1,5 @@
> +{
> +   "files.associations": {
> +      "amfenc.h": "c"
> +   }
> +}
> \ No newline at end of file
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index d53b8ff..32d147f 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -266,6 +266,8 @@ OBJS-$(CONFIG_DIRAC_DECODER)           += diracdec.o dirac.o diracdsp.o diractab
>  OBJS-$(CONFIG_DFA_DECODER)             += dfa.o
>  OBJS-$(CONFIG_DNXHD_DECODER)           += dnxhddec.o dnxhddata.o
>  OBJS-$(CONFIG_DNXHD_ENCODER)           += dnxhdenc.o dnxhddata.o
> +OBJS-$(CONFIG_M264_ENCODER)            += m264enc.o
> +OBJS-$(CONFIG_M264_DECODER)            += m264dec.o
>  OBJS-$(CONFIG_DOLBY_E_DECODER)         += dolby_e.o kbdwin.o
>  OBJS-$(CONFIG_DPX_DECODER)             += dpx.o
>  OBJS-$(CONFIG_DPX_ENCODER)             += dpxenc.o
> @@ -1093,6 +1095,9 @@ OBJS-$(CONFIG_VP9_RAW_REORDER_BSF)        += vp9_raw_reorder_bsf.o
>  OBJS-$(CONFIG_VP9_SUPERFRAME_BSF)         += vp9_superframe_bsf.o
>  OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF)   += vp9_superframe_split_bsf.o
>  

> +
> +
> +

Unrelated. Re-read your patch before sending it.

>  # thread libraries
>  OBJS-$(HAVE_LIBC_MSVCRT)               += file_open.o
>  OBJS-$(HAVE_THREADS)                   += pthread.o pthread_slice.o pthread_frame.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index d70646e..e4a6721 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -84,6 +84,8 @@ extern AVCodec ff_dfa_decoder;
>  extern AVCodec ff_dirac_decoder;
>  extern AVCodec ff_dnxhd_encoder;
>  extern AVCodec ff_dnxhd_decoder;
> +extern AVCodec ff_m264_encoder;
> +extern AVCodec ff_m264_decoder;
>  extern AVCodec ff_dpx_encoder;
>  extern AVCodec ff_dpx_decoder;
>  extern AVCodec ff_dsicinvideo_decoder;
> @@ -800,8 +802,8 @@ const AVCodec *av_codec_iterate(void **opaque)
>      ff_thread_once(&av_codec_static_init, av_codec_init_static);
>  
>      if (c)
> -        *opaque = (void*)(i + 1);
> -
> +      *opaque = (void*)(i + 1);
> +      
>      return c;
>  }
>  
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 3922e89..a1c6245 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -452,6 +452,7 @@ enum AVCodecID {
>      AV_CODEC_ID_MWSC,
>      AV_CODEC_ID_WCMV,
>      AV_CODEC_ID_RASC,

> +    AV_CODEC_ID_M264,

I thought this was supposed to be H.264.

>  
>      /* various PCM "codecs" */
>      AV_CODEC_ID_FIRST_AUDIO = 0x10000,     ///< A dummy id pointing at the start of audio codecs
> @@ -2978,6 +2979,8 @@ typedef struct AVCodecContext {
>  #define FF_PROFILE_PRORES_4444      4
>  #define FF_PROFILE_PRORES_XQ        5
>  
> +#define FF_PROFILE_M264             0
> +
>      /**
>       * level
>       * - encoding: Set by user.
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 4850e4f..53859cd 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -1691,6 +1691,15 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .long_name = NULL_IF_CONFIG_SMALL("RemotelyAnywhere Screen Capture"),
>          .props     = AV_CODEC_PROP_LOSSY,
>      },
> +    
> +    {

> +        .id        = AV_CODEC_ID_H264,

Unacceptable, this table is meant to contain one entry per codec.

> +        .type      = AVMEDIA_TYPE_VIDEO,
> +        .name      = "m264",
> +        .long_name = NULL_IF_CONFIG_SMALL("Matrox M264 Codec"),
> +        .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_REORDER,
> +        .profiles  = NULL_IF_CONFIG_SMALL(ff_h264_profiles),
> +    },
>  
>      /* various PCM "codecs" */
>      {
> diff --git a/libavcodec/m264dec.c b/libavcodec/m264dec.c
> new file mode 100644
> index 0000000..e1547e8
> --- /dev/null
> +++ b/libavcodec/m264dec.c
> @@ -0,0 +1,476 @@
> +/*
> + * M264 H.264 video decoder
> + * Copyright (C) 2019 Matrox
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file

> + * Matrox M264 card supports h.264 encoding and decoding.

If it is hardware-accelerated code, there are naming conventions to
respect.

If it is not, then you need to explain the benefits of this code.

> + */
> +
> +#include "decode.h"
> +#include "cabac.h"
> +#include "error_resilience.h"
> +#include "h264_parse.h"
> +#include "h264_ps.h"
> +#include "h264_sei.h"
> +#include "h2645_parse.h"
> +#include "h264chroma.h"
> +#include "h264dsp.h"
> +#include "h264pred.h"
> +#include "h264qpel.h"
> +#include "internal.h"
> +#include "mpegutils.h"
> +#include "parser.h"
> +#include "qpeldsp.h"
> +#include "rectangle.h"
> +#include "videodsp.h"
> +
> +#include "config.h"
> +#include "libswscale/swscale.h"
> +
> +#include "m264dec.h"
> +
> +#ifdef _WIN32
> +#include "compat/w32dlfcn.h"
> +#else
> +#include <dlfcn.h>
> +#endif
> +
> +typedef struct
> +{
> +   uint16_t width;
> +   uint16_t height;
> +   uint16_t  scale;
> +   uint16_t  rate;
> +   uint16_t gopsize;
> +   uint32_t bitrate;
> +   uint8_t  field_order;
> +} M264DecoderInfo;
> +
> +typedef struct
> +{
> +   int (*init_m264_decoder)(void *decoder_info, void **m264_decoder_object);
> +   int (*exit_m264_decoder)(void *m264_decoder_object);
> +   int (*send_packet)(void *m264_decoder_object, void *extradata, unsigned long extradata_size, void *buffer, unsigned long buffer_size);
> +   int (*receive_frame)(void *m264_decoder_object, void **buffer_context, void **buffer, unsigned long *buffer_size);
> +   int (*release_frame_buffer)(void *m264_decoder_object, void *buffer_context);
> +   void *lib_handle;
> +   void *decoder_context;
> +   int64_t frames;
> +   int eof;
> +   char *sps_pps;
> +   int length_sps_pps;
> +} M264Decoder;
> +

> +void convert_to_annexb(unsigned char * dest, unsigned long data_size)

Probably duplicated.

> +{
> +   unsigned char *current = dest;
> +   union 
> +   {
> +      unsigned char by4[4];
> +      uint32_t length;
> +   } union_u32_byte;
> +  
> +   while ((dest + data_size) > (current + 4))
> +   {
> +      if((current[0] == 0) &&
> +         (current[1] == 0) &&
> +         (current[2] == 0) &&
> +         (current[3] == 1))
> +      {
> +         // in case it is already in annex B
> +         break;
> +      }
> +
> +      union_u32_byte.by4[3] = current[0];
> +      union_u32_byte.by4[2] = current[1];
> +      union_u32_byte.by4[1] = current[2];
> +      union_u32_byte.by4[0] = current[3];
> +
> +      current[0] = 0;
> +      current[1] = 0;
> +      current[2] = 0;
> +      current[3] = 1;
> +
> +      if( union_u32_byte.length <= (data_size - 4 ) )
> +         current += (union_u32_byte.length + 4);
> +      else
> +         break;
> +   }
> +}
> +

> +unsigned short convert_bytes_to_ushort(unsigned char * bytes, unsigned char ** bytes_next)

Definitely redundant.

> +{
> +   union 
> +   {
> +      unsigned char by2[2];
> +      unsigned short ushort;
> +   } union_short_byte;
> +   
> +   union_short_byte.by2[1] = bytes[0];
> +   union_short_byte.by2[0] = bytes[1];
> +
> +   if (bytes_next)
> +      *bytes_next = bytes + 2;
> +
> +   return union_short_byte.ushort;
> +};
> +
> +
> +int ff_m264_init_spspps_mp4(AVCodecContext *avctx)
> +{
> +   M264Decoder *m264_decoder = (M264Decoder *)avctx->priv_data;
> +   unsigned char configuration_version;
> +   unsigned char avc_profile_indication;
> +   unsigned char profile_compatibility;
> +   unsigned char avc_level_indication;
> +
> +   unsigned char size_field_in_bytes;
> +
> +   unsigned char sps_count = 0;
> +
> +   unsigned short * array_sps_length = NULL;
> +   unsigned char ** array_sps_nal_unit = NULL;
> +
> +   unsigned char pps_count = 0;
> +   unsigned short * array_pps_length = NULL;
> +   unsigned char ** array_pps_nal_unit = NULL;
> +
> +   unsigned char * current;
> +
> +   unsigned long totalsize = 0;
> +
> +   current = avctx->extradata;
> +   configuration_version = *current++;
> +   
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_init_spspps_mp4: configuration_version = %d\n", configuration_version);
> +   assert(configuration_version == 1);
> +
> +   avc_profile_indication = *current++;
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_init_spspps_mp4: avc_profile_indication = %d\n", avc_profile_indication);
> +
> +   profile_compatibility = *current++;
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_init_spspps_mp4: profile_compatibility = %d\n", profile_compatibility);
> +
> +   avc_level_indication = *current++;
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_init_spspps_mp4: avc_level_indication = %d\n", avc_level_indication);
> +
> +   size_field_in_bytes = *current++;
> +
> +   assert((size_field_in_bytes & 0xFC) == 0xFC);
> +
> +   size_field_in_bytes &= 0x03;
> +
> +   size_field_in_bytes += 1;
> +
> +   sps_count = *current++;
> +
> +   sps_count &= 0x1F;
> +
> +   if (sps_count == 0)
> +   {
> +      // no valid sps pps
> +      return 0;
> +   }
> +
> +   array_sps_length = (unsigned short*) malloc(sps_count * sizeof (unsigned short));
> +   array_sps_nal_unit = (unsigned char**) malloc(sps_count * sizeof (unsigned char *));
> +
> +   for (unsigned long i=0; i< sps_count; i++) 
> +   {
> +      array_sps_length[i] = convert_bytes_to_ushort(current, &current);
> +      array_sps_nal_unit[i] = (unsigned char*)malloc(array_sps_length[i]);
> +      memcpy(array_sps_nal_unit[i], current, array_sps_length[i]);
> +      current += array_sps_length[i];
> +
> +      totalsize += 4 + array_sps_length[i];
> +   }
> +
> +   pps_count = *current++;
> +
> +   array_pps_length = (unsigned short*)malloc(pps_count * sizeof (unsigned short));
> +   array_pps_nal_unit = (unsigned char**) malloc(pps_count * sizeof (unsigned char *));
> +
> +   for (unsigned long i=0; i< pps_count; i++) 
> +   {
> +      array_pps_length[i] = convert_bytes_to_ushort(current, &current);
> +      array_pps_nal_unit[i] = (unsigned char*)malloc(array_pps_length[i]);
> +      memcpy(array_pps_nal_unit[i], current, array_pps_length[i]);
> +      current += array_pps_length[i];
> +      totalsize += 4 + array_pps_length[i];
> +   }
> +
> +   m264_decoder->sps_pps = av_mallocz(totalsize);
> +   current = m264_decoder->sps_pps;
> +
> +   current[0] = 0;
> +   current[1] = 0;
> +   current[2] = 0;
> +   current[3] = 1;
> +   current += 4;
> +
> +   memcpy(current, array_sps_nal_unit[0], array_sps_length[0]);
> +   current +=  array_sps_length[0];
> +
> +   current[0] = 0;
> +   current[1] = 0;
> +   current[2] = 0;
> +   current[3] = 1;
> +
> +   current += 4;
> +
> +   memcpy(current, array_pps_nal_unit[0], array_pps_length[0]);
> +   current +=  array_pps_length[0];
> +
> +   m264_decoder->length_sps_pps = totalsize;
> +
> +   return 0;
> +}
> +
> +av_cold int ff_m264_decode_init(AVCodecContext *avctx)
> +{
> +   int result = 0;
> +   void *lib_handle;
> +   M264Decoder *m264_decoder;
> +   M264DecoderInfo decoder_info;
> +
> +   #ifdef _WIN32
> +   av_log(avctx, AV_LOG_DEBUG, "_WIN32\n");
> +   #elif defined _WIN64
> +   av_log(avctx, AV_LOG_DEBUG, "_WIN64\n");
> +   #else
> +   av_log(avctx, AV_LOG_DEBUG, "linux\n");
> +   #endif
> +
> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif
> +
> +   if (!lib_handle)
> +   {
> +      av_log(avctx, AV_LOG_ERROR, "failed to load mvM264ffmpeg\n");
> +   }
> +
> +   m264_decoder =  av_mallocz(sizeof(M264Decoder));
> +
> +   m264_decoder->init_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_init");
> +   m264_decoder->exit_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_exit");
> +   m264_decoder->send_packet = dlsym(lib_handle, "m264_ffmpeg_decoder_send_packet");
> +   m264_decoder->receive_frame = dlsym(lib_handle, "m264_ffmpeg_decoder_receive_frame");
> +   m264_decoder->release_frame_buffer = dlsym(lib_handle, "m264_ffmpeg_decoder_release_frame_buffer");
> +   m264_decoder->lib_handle = lib_handle;
> +
> +   m264_decoder->eof = 0;
> +
> +   decoder_info.width = avctx->width;
> +   decoder_info.height = avctx->height;
> +   decoder_info.scale = avctx->framerate.num;
> +   decoder_info.rate = avctx->framerate.den;
> +
> +   switch (avctx->field_order)
> +   {
> +      case AV_FIELD_UNKNOWN: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_UNKNOWN \n");
> +         break;
> +      case AV_FIELD_PROGRESSIVE: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_PROGRESSIVE \n");
> +         break;
> +      case AV_FIELD_TT: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TT \n");
> +         break;
> +      case AV_FIELD_BB: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BB \n");
> +         break;
> +      case AV_FIELD_TB: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TB \n");
> +         break;
> +      case AV_FIELD_BT: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BT \n");
> +         break;
> +      default:
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is default \n");
> +         assert(false);
> +         break;
> +   }
> +
> +   decoder_info.field_order = (uint8_t)avctx->field_order;
> +
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->width = %d\n", avctx->width);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->height = %d\n", avctx->height);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);
> +
> +   result = m264_decoder->init_m264_decoder(&decoder_info, &(m264_decoder->decoder_context));
> +
> +   avctx->priv_data = m264_decoder;
> +
> +   avctx->pix_fmt = AV_PIX_FMT_YUYV422;
> +
> +   avctx->bits_per_raw_sample = 8;     
> +
> +   if (avctx->extradata_size > 0 && avctx->extradata)
> +   {
> +      ff_m264_init_spspps_mp4(avctx);
> +   }
> +
> +   return result;
> +}
> +
> +int ff_m264_receive_frame(AVCodecContext *avctx, AVFrame *frame)
> +{
> +   int ret = 0;
> +   AVPacket pkt = {0};
> +   void *buffer_context = NULL;
> +   void *buffer = NULL;
> +   unsigned long buffer_size = 0;
> +
> +   unsigned long line_size;
> +
> +   int line;
> +
> +   int actual_line_size;
> +
> +   unsigned char *srcmem,*dstmem;
> +
> +   M264Decoder *m264_decoder = (M264Decoder *)avctx->priv_data;
> +
> +   ret = m264_decoder->receive_frame( m264_decoder->decoder_context, &buffer_context, &buffer, &buffer_size);
> +
> +   if (ret == -1)
> +   {
> +      av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_frame: m264_decoder->receive_packet get eos, return AVERROR_EOF \n");
> +      return AVERROR_EOF;
> +   }
> +
> +   if (buffer_size > 0)
> +   {
> +      av_packet_unref(&pkt);
> +
> +      ret = ff_get_buffer(avctx, frame, 0);
> +
> +      line_size = buffer_size / avctx->height;
> +
> +      srcmem = (unsigned char *)buffer;
> +      dstmem = (unsigned char *)frame->data[0];
> +
> +      actual_line_size = avctx->width * 2;
> +
> +      for (line = 0; line < avctx->height; line++)
> +      {
> +         memcpy(dstmem, srcmem, actual_line_size);
> +         dstmem += frame->linesize[0];
> +         srcmem += line_size;
> +      }
> +
> +      frame->pts = m264_decoder->frames;
> +
> +      m264_decoder->frames++;
> +
> +      av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_frame: m264_decoder->frames = %d\n", (int)m264_decoder->frames);
> +      ret = m264_decoder->release_frame_buffer(m264_decoder->decoder_context, buffer_context);
> +   }
> +   else
> +   {
> +      if( m264_decoder->eof != 1 )
> +      {
> +         ret = ff_decode_get_packet(avctx, &pkt);
> +
> +         av_log(avctx, AV_LOG_DEBUG, "decoder: ff_m264_receive_frame: ff_decode_get_packet  pkt.size = %d\n", pkt.size);
> +
> +         if (ret < 0 && ret != AVERROR_EOF) {
> +            return ret;
> +         }
> +      }
> +
> +      if (ret == AVERROR_EOF )
> +      {
> +         if( m264_decoder->eof != 1 )
> +         {
> +            pkt.data = NULL;
> +            pkt.size = 0;
> +            m264_decoder->eof = 1;
> +            ret = m264_decoder->send_packet(m264_decoder->decoder_context, m264_decoder->sps_pps, m264_decoder->length_sps_pps, pkt.data, pkt.size);
> +         }
> +      }
> +      else 
> +      {
> +         if( m264_decoder->eof != 1 )
> +         {
> +            av_log(avctx, AV_LOG_DEBUG, "decoder: ff_m264_receive_frame: pkt.side_data_elems = %d\n", pkt.side_data_elems);
> +
> +            if (pkt.data != NULL)
> +            {
> +               convert_to_annexb( pkt.data, pkt.size);
> +            }   
> +
> +            if (pkt.flags)
> +            {
> +               ret = m264_decoder->send_packet(m264_decoder->decoder_context, m264_decoder->sps_pps, m264_decoder->length_sps_pps, pkt.data, pkt.size);
> +            }
> +            else
> +            {
> +               ret = m264_decoder->send_packet(m264_decoder->decoder_context, NULL , 0, pkt.data, pkt.size);
> +            }
> +         }
> +      }
> +
> +      ret = AVERROR(EAGAIN);
> +   }
> +   return ret;
> +}
> +
> +
> +int av_cold ff_m264_decode_close(AVCodecContext *avctx)
> +{
> +   M264Decoder *m264_decoder = (M264Decoder *)avctx->priv_data;
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_decode_close: 1 \n");
> +
> +   m264_decoder->exit_m264_decoder(m264_decoder->decoder_context);
> +
> +   dlclose(m264_decoder->lib_handle);   
> +
> +   av_free(m264_decoder->sps_pps);
> +
> +   av_free(m264_decoder);
> +
> +   avctx->priv_data = NULL;
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_decode_close: 2 \n");
> +
> +   return 0;
> +}
> +
> +
> +AVCodec ff_m264_decoder = {
> +    .name           = "m264",
> +    .long_name      = NULL_IF_CONFIG_SMALL("Matrox M264 Codec"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_H264,
> +    .init           = ff_m264_decode_init,
> +    .receive_frame   = ff_m264_receive_frame,
> +    .close          = ff_m264_decode_close,
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> +    .wrapper_name   = "m264",
> +};
> +
> diff --git a/libavcodec/m264dec.h b/libavcodec/m264dec.h
> new file mode 100644
> index 0000000..ee832fe
> --- /dev/null
> +++ b/libavcodec/m264dec.h
> @@ -0,0 +1,42 @@
> +/*
> + * M264 H.264 video decoder
> + * Copyright (C) 2019 Matrox
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * Matrox M264 card supports h.264 encoding and decoding.
> + */
> +
> +#ifndef AVCODEC_M264DEC_H
> +#define AVCODEC_M264DEC_H
> +
> +#include "libavutil/fifo.h"
> +
> +#include "avcodec.h"
> +
> +int ff_m264_decode_init(AVCodecContext *avctx);
> +int ff_m264_decode_close(AVCodecContext *avctx);
> +int ff_m264_receive_frame(AVCodecContext *avctx, AVFrame *frame);
> +int ff_m264_init_spspps_mp4(AVCodecContext *avctx);
> +unsigned short convert_bytes_to_ushort(unsigned char * bytes, unsigned char ** next_bytes);
> +void convert_to_annexb(unsigned char * dest, unsigned long datasize);
> +
> +#endif //AVCODEC_M264DEC_H
> +
> diff --git a/libavcodec/m264enc.c b/libavcodec/m264enc.c
> new file mode 100644
> index 0000000..43a95e2
> --- /dev/null
> +++ b/libavcodec/m264enc.c
> @@ -0,0 +1,289 @@
> +/*
> + * M264 H.264 video encoder
> + * Copyright (C) 2019 Matrox
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * Matrox M264 card supports h.264 encoding and decoding.
> + */
> +
> +#include "libavutil/avassert.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/channel_layout.h"
> +#include "libavutil/opt.h"
> +#include "config.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/imgutils.h"
> +#include "libavutil/hwcontext.h"
> +#if CONFIG_D3D11VA
> +#include "libavutil/hwcontext_d3d11va.h"
> +#endif
> +#if CONFIG_DXVA2
> +#define COBJMACROS
> +#include "libavutil/hwcontext_dxva2.h"
> +#endif
> +#include "libavutil/mem.h"
> +#include "libavutil/pixdesc.h"
> +#include "libavutil/time.h"
> +#include "internal.h"
> +
> +#if CONFIG_D3D11VA
> +#include <d3d11.h>
> +#endif
> +
> +#include "libswscale/swscale.h"
> +#include "m264enc.h"
> +#include "internal.h"
> +
> +#ifdef _WIN32
> +#include "compat/w32dlfcn.h"
> +#else
> +#include <dlfcn.h>
> +#endif
> +
> +#include "m264enc.h"
> +
> +typedef struct
> +{
> +   uint16_t width;
> +   uint16_t height;
> +   uint16_t  scale;
> +   uint16_t  rate;
> +   uint16_t gop_size;
> +   uint32_t bitrate;
> +} M264EncoderInfo;
> +
> +typedef struct
> +{
> +   int (*init_m264)(void *encoder_info, void **m264_encoder_object);
> +   int (*exit_m264)(void *m264_encoder_object);
> +
> +   int (*get_uncompressed_buffer)(void *m264_encoder_object, void **buffer_context, void **buffer, unsigned long *buffer_size);
> +   int (*send_frame)(void *m264_encoder_object, void *buffer_context, int polarity);
> +   int (*receive_packet)(void *m264_encoder_object, void *buffer_context, void **buffer, unsigned long *buffer_size, int32_t *pts, int32_t *dts, int8_t *key);
> +   int (*release_packet_buffer)(void *m264_encoder_object, void *buffer_context);
> +   void *lib_handle;
> +
> +   void *encoder_context;
> +   struct SwsContext *sw_context;
> +} M264Encoder;
> +
> +av_cold int ff_m264_encode_init(AVCodecContext *avctx)
> +{
> +   int result = 0;
> +   void *lib_handle;
> +   M264Encoder *m264_encoder;
> +   M264EncoderInfo encoder_info;
> +
> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif
> +
> +   if (!lib_handle)
> +   {
> +      av_log(avctx, AV_LOG_ERROR, "failed to load mvM264ffmpeg\n");
> +   }
> +
> +   m264_encoder =  av_mallocz(sizeof(M264Encoder));
> +
> +   m264_encoder->init_m264 = dlsym(lib_handle, "m264_ffmpeg_encoder_init");
> +   m264_encoder->exit_m264 = dlsym(lib_handle, "m264_ffmpeg_encoder_exit");
> +   m264_encoder->get_uncompressed_buffer = dlsym(lib_handle, "m264_ffmpeg_encoder_get_uncompressed_buffer");
> +   m264_encoder->send_frame = dlsym(lib_handle, "m264_ffmpeg_encoder_send_frame");
> +   m264_encoder->receive_packet = dlsym(lib_handle, "m264_ffmpeg_encoder_receive_packet");
> +   m264_encoder->release_packet_buffer = dlsym(lib_handle, "m264_ffmpeg_encoder_release_packet_buffer");
> +   m264_encoder->lib_handle = lib_handle;
> +
> +   encoder_info.width = avctx->width;
> +   encoder_info.height = avctx->height;
> +   encoder_info.rate = avctx->framerate.num;
> +   encoder_info.scale = avctx->framerate.den;
> +   encoder_info.gop_size = avctx->gop_size;
> +   encoder_info.bitrate = avctx->bit_rate;
> +
> +   printf("m264_encode_init_h264: avctx->width = %d\n", avctx->width);
> +   printf("m264_encode_init_h264: avctx->height = %d\n", avctx->height);
> +   printf("m264_encode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   printf("m264_encode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);
> +   printf("m264_encode_init_h264: avctx->gop_size = %d\n", avctx->gop_size);
> +   printf("m264_encode_init_h264: avctx->bit_rate = %" PRId64 "\n", avctx->bit_rate);
> +
> +   result = m264_encoder->init_m264(&encoder_info, &(m264_encoder->encoder_context));
> +
> +   avctx->priv_data = m264_encoder;
> +
> +   return result;
> +}
> +
> +int ff_m264_send_frame(AVCodecContext *avctx, const AVFrame *frame)
> +{
> +   int result = 0;
> +   enum AVPixelFormat src_pixel_format;
> +   enum AVPixelFormat dest_pixel_format;
> +   uint8_t *dst[1];
> +   int dst_stride[1];
> +   void *buffer_context;
> +
> +   unsigned long buffer_size;
> +
> +   unsigned long line_size;
> +
> +   int line;
> +
> +   int actual_line_size;
> +
> +   unsigned char *srcmem,*dstmem;
> +
> +   M264Encoder *m264_encoder = (M264Encoder *)avctx->priv_data;
> +
> +   if (frame)
> +   {
> +      src_pixel_format = (enum AVPixelFormat)frame->format;
> +      dest_pixel_format = AV_PIX_FMT_YUYV422;
> +
> +      if (m264_encoder->sw_context == NULL)
> +      {
> +         m264_encoder->sw_context = sws_getContext(frame->width, frame->height, src_pixel_format, frame->width, frame->height,
> +                                    dest_pixel_format, SWS_BILINEAR, NULL, NULL, NULL);
> +         if (!m264_encoder->sw_context) {
> +            av_log(avctx, AV_LOG_ERROR, "failed to load mvm264ffmpeg.so \n");
> +            result = -1;
> +            return result;
> +         }
> +      }
> +
> +      result = m264_encoder->get_uncompressed_buffer(m264_encoder->encoder_context, &buffer_context, (void **)&dst[0], &buffer_size);
> +
> +      dst_stride[0] = buffer_size / avctx->height;
> +      if ( src_pixel_format != AV_PIX_FMT_YUYV422 )
> +      {
> +         result = sws_scale(m264_encoder->sw_context, (const uint8_t * const*)frame->data, frame->linesize, 0, frame->height, dst, dst_stride);
> +      }
> +      else
> +      {
> +         line_size = buffer_size / avctx->height;
> +
> +         dstmem = (unsigned char *)dst[0];
> +         srcmem = (unsigned char *)frame->data[0];
> +
> +         actual_line_size = avctx->width * 2;
> +
> +         for (line = 0; line < avctx->height; line++)
> +         {
> +            memcpy(dstmem, srcmem, actual_line_size);
> +            srcmem += frame->linesize[0];
> +            dstmem += line_size;
> +         }
> +      }
> +
> +      av_log(avctx, AV_LOG_DEBUG, "ff_m264_send_frame: frame->width = %d, frame->height = %d\n", frame->width, frame->height);
> +
> +      result = m264_encoder->send_frame(m264_encoder->encoder_context, buffer_context, 0);
> +   }
> +   else
> +   {
> +      av_log(avctx, AV_LOG_DEBUG, "ff_m264_send_frame: frame is null. eof\n");
> +      result = m264_encoder->send_frame(m264_encoder->encoder_context, NULL, 0);
> +   }
> +
> +   return result;
> +}
> +
> +int ff_m264_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
> +{
> +   int result;
> +   M264Encoder *m264_encoder = (M264Encoder *)avctx->priv_data;
> +
> +   void *buffer_context = NULL;
> +   void *buffer = NULL;
> +   unsigned long buffer_size = 0;
> +
> +   int32_t pts, dts;
> +   int8_t key;
> +
> +   result = m264_encoder->receive_packet(m264_encoder->encoder_context, &buffer_context, &buffer, &buffer_size, &pts, &dts, &key);
> +
> +   if (result == -1)
> +   {
> +      av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_packet: m264_encoder->receive_packet get eos, return AVERROR_EOF\n");
> +      return AVERROR_EOF;
> +   }
> +   else if (buffer_size == 0)
> +   {
> +      return AVERROR(EAGAIN);
> +   }
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_packet  m264_encoder->receive_packet buffer_size = %d\n", (int)buffer_size); 
> +
> +   result = ff_alloc_packet2(avctx, avpkt, buffer_size, 0);
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_packet  ff_alloc_packet2 result = %d\n", result); 
> +   memcpy(avpkt->data, buffer, buffer_size);
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_packet  memcpy done\n"); 
> +
> +   avpkt->dts = dts;
> +   avpkt->pts = pts + 2;
> +   avpkt->flags = key ? AV_PKT_FLAG_KEY : 0;
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_packet  m264_encoder->release_packet_buffer before \n"); 
> +   result = m264_encoder->release_packet_buffer(m264_encoder->encoder_context, buffer_context);
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_receive_packet  m264_encoder->release_packet_buffer after \n"); 
> +
> +   return result;
> +}
> +
> +int av_cold ff_m264_encode_close(AVCodecContext *avctx)
> +{
> +   M264Encoder *m264_encoder = (M264Encoder *)avctx->priv_data;
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_encode_close 1");
> +   
> +   m264_encoder->exit_m264(m264_encoder->encoder_context);
> +
> +   sws_freeContext(m264_encoder->sw_context);
> +
> +   dlclose(m264_encoder->lib_handle);   
> +
> +   av_free(m264_encoder);
> +
> +   avctx->priv_data = NULL;
> +
> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_encode_close 2");
> +
> +   return 0;
> +}
> +
> +AVCodec ff_m264_encoder = {
> +    .name           = "m264",
> +    .long_name      = NULL_IF_CONFIG_SMALL("Matrox M264 Codec"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_H264,
> +    .init           = ff_m264_encode_init,
> +    .send_frame     = ff_m264_send_frame,
> +    .receive_packet = ff_m264_receive_packet,
> +    .close          = ff_m264_encode_close,
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> +    .wrapper_name   = "m264",
> +};
> +
> +
> diff --git a/libavcodec/m264enc.h b/libavcodec/m264enc.h
> new file mode 100644
> index 0000000..63718e6
> --- /dev/null
> +++ b/libavcodec/m264enc.h
> @@ -0,0 +1,39 @@
> +/*
> + * M264 H.264 video encoder
> + * Copyright (C) 2019 Matrox
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * Matrox M264 card supports h.264 encoding and decoding.
> + */
> +
> +#ifndef AVCODEC_M264ENC_H
> +#define AVCODEC_M264ENC_H
> +
> +#include "libavutil/fifo.h"
> +
> +#include "avcodec.h"
> +
> +int ff_m264_encode_init(AVCodecContext *avctx);
> +int ff_m264_encode_close(AVCodecContext *avctx);
> +int ff_m264_send_frame(AVCodecContext *avctx, const AVFrame *frame);
> +int ff_m264_receive_packet(AVCodecContext *avctx, AVPacket *avpkt);
> +
> +#endif //AVCODEC_AMAVCODEC_M264ENC_H
> diff --git a/libavcodec/profiles.c b/libavcodec/profiles.c
> index e6f937f..1c161f9 100644
> --- a/libavcodec/profiles.c
> +++ b/libavcodec/profiles.c
> @@ -170,4 +170,9 @@ const AVProfile ff_mjpeg_profiles[] = {
>      { FF_PROFILE_UNKNOWN }
>  };
>  
> +const AVProfile ff_m264_profiles[] = {
> +  { FF_PROFILE_M264,       "M264"},
> +};
> +
> +
>  #endif /* !CONFIG_SMALL */
> diff --git a/libavcodec/profiles.h b/libavcodec/profiles.h
> index ab61e03..9eafda2 100644
> --- a/libavcodec/profiles.h
> +++ b/libavcodec/profiles.h
> @@ -35,5 +35,6 @@ extern const AVProfile ff_av1_profiles[];
>  extern const AVProfile ff_sbc_profiles[];
>  extern const AVProfile ff_prores_profiles[];
>  extern const AVProfile ff_mjpeg_profiles[];

> +extern const AVProfile ff_m264_profiles[];

Unused.

>  
>  #endif /* AVCODEC_PROFILES_H */

Regards,

Hi

> +void convert_to_annexb(unsigned char * dest, unsigned long
> data_size)
> +{
> +   unsigned char *current = dest;
> +   union 
> +   {
> +      unsigned char by4[4];
> +      uint32_t length;
> +   } union_u32_byte;
> +  
> +   while ((dest + data_size) > (current + 4))
> +   {
> +      if((current[0] == 0) &&
> +         (current[1] == 0) &&
> +         (current[2] == 0) &&
> +         (current[3] == 1))
> +      {
> +         // in case it is already in annex B
> +         break;
> +      }
> +
> +      union_u32_byte.by4[3] = current[0];
> +      union_u32_byte.by4[2] = current[1];
> +      union_u32_byte.by4[1] = current[2];
> +      union_u32_byte.by4[0] = current[3];

This won't work on certain machines. Type punning is also undefined if
memory serves me right, and may not work right in all compilers.
Definitely not recommended. There are macros in FFmpeg for this already
(AV_RL32 and friends)

> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif

I believe FFmpeg has policy specifically against stuff like this. Link
the dynamic library properly, and put appropriate license stuff in
configure. The resulting ffmpeg binaries will more than likely not be
redistributable, libav* might also be depending on what the Matrox
library's license is.

> +   printf("m264_encode_init_h264: avctx->width = %d\n", avctx->width);
> +   printf("m264_encode_init_h264: avctx->height = %d\n", avctx->height);
> +   printf("m264_encode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   printf("m264_encode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);
> +   printf("m264_encode_init_h264: avctx->gop_size = %d\n", avctx->gop_size);
> +   printf("m264_encode_init_h264: avctx->bit_rate = %" PRId64 "\n", avctx->bit_rate);

Forgotten debug stuff? printf() is definitely not OK

There are other issues with this patch, but there's no point in looking
more for now

/Tomas

Hi Yufei,
before providing large patches here, do read this mailing list and the
ffmpeg codebase for a while. It will help you to understand the
process, and to understand how ffmpeg's sources are organized.

I'm sure you missed this:
https://www.ffmpeg.org/developer.html

Read all of it thoroughly.

The content of this section:
https://www.ffmpeg.org/developer.html#Coding-Rules-1
especially comes to mind. Your code uses totally different formatting
than the rest of the ffmpeg codebase. You should take a look around as
see how others do it, and what that style guide says.

Apart from that: Everything that Nicolas wrote.

In addition this:

On Mon, Feb 25, 2019 at 19:49:43 +0000, Yufei He wrote:
> index c90f119..f70368b 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -11,6 +11,7 @@ version <next>:
>  - dhav demuxer
>  - PCM-DVD encoder
>  - GIF parser
> +- M264 encoder

Your patch is against an at least two months old version of ffmpeg git.
Please merge it to latest git HEAD and create a new patch. Your patch
won't apply currently, and therefore noone will bother to test it.

And even if I try to work around that, here's what happens:

LD      ffmpeg_g
/usr/bin/ld: libavcodec/libavcodec.a(m264enc.o): in function `ff_m264_encode_init':
/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:98: undefined reference to `dlopen'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:108: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:109: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:110: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:111: undefined reference to `dlsym'
/usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:112: undefined reference to `dlsym'
/usr/bin/ld: libavcodec/libavcodec.a(m264enc.o):/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:113: more undefined references to `dlsym' follow
/usr/bin/ld: libavcodec/libavcodec.a(m264enc.o): in function `ff_m264_encode_close':
/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:264: undefined reference to `dlclose'
collect2: error: ld returned 1 exit status
make: *** [Makefile:108: ffmpeg_g] Error 1

You need to get your dependencies right.

> +   if(*got_output)        
> +   {
> +      if(decoded_frame->width == 0)
> +      {
> +         av_log(NULL, AV_LOG_DEBUG, "Frame parameters mismatch context %d,%d,%d != %d,%d,%d\n",
> +               decoded_frame->width,
> +               decoded_frame->height,
> +               decoded_frame->format,
> +               ist->dec_ctx->width,
> +               ist->dec_ctx->height,
> +               ist->dec_ctx->pix_fmt);
> +      }
> +   }

This is debug code and does not belong into a released codec.
Furthermore, ffmpeg provides av_log() functions.

> index 0000000..dc1852f
> --- /dev/null
> +++ b/libavcodec/.vscode/settings.json

Don't commit your local development environment's settings, please.

>  OBJS-$(CONFIG_DNXHD_DECODER)           += dnxhddec.o dnxhddata.o
>  OBJS-$(CONFIG_DNXHD_ENCODER)           += dnxhdenc.o dnxhddata.o
> +OBJS-$(CONFIG_M264_ENCODER)            += m264enc.o
> +OBJS-$(CONFIG_M264_DECODER)            += m264dec.o
>  OBJS-$(CONFIG_DOLBY_E_DECODER)         += dolby_e.o kbdwin.o
>  OBJS-$(CONFIG_DPX_DECODER)             += dpx.o

Do you realize that the rest of this list is in alphabetical order?

>  OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF)   += vp9_superframe_split_bsf.o
>  
> +
> +
> +
>  # thread libraries

Why do you add useless empty lines, and commit them?

>  extern AVCodec ff_dnxhd_encoder;
>  extern AVCodec ff_dnxhd_decoder;
> +extern AVCodec ff_m264_encoder;
> +extern AVCodec ff_m264_decoder;
>  extern AVCodec ff_dpx_encoder;
>  extern AVCodec ff_dpx_decoder;

Alphabetical order, again.

>      if (c)
> -        *opaque = (void*)(i + 1);
> -
> +      *opaque = (void*)(i + 1);
> +      
>      return c;

Useless change. And please don't ever leave whitespace at your line
endings, it won't be accepted. (Your IDE surely can fix this for you.)

> +#define FF_PROFILE_M264             0

What is this?

> +        .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_REORDER,
> +        .profiles  = NULL_IF_CONFIG_SMALL(ff_h264_profiles),

Does the encoder even honor any of the profiles?

> +   #ifdef _WIN32
> +   av_log(avctx, AV_LOG_DEBUG, "_WIN32\n");
> +   #elif defined _WIN64
> +   av_log(avctx, AV_LOG_DEBUG, "_WIN64\n");
> +   #else
> +   av_log(avctx, AV_LOG_DEBUG, "linux\n");
> +   #endif

What for?

> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif

I'm not sure this is acceptable within ffmpeg.

> +   if (!lib_handle)
> +   {
> +      av_log(avctx, AV_LOG_ERROR, "failed to load mvM264ffmpeg\n");
> +   }
> +
> +   m264_decoder =  av_mallocz(sizeof(M264Decoder));
> +
> +   m264_decoder->init_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_init");
> +   m264_decoder->exit_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_exit");
> +   m264_decoder->send_packet = dlsym(lib_handle, "m264_ffmpeg_decoder_send_packet");
> +   m264_decoder->receive_frame = dlsym(lib_handle, "m264_ffmpeg_decoder_receive_frame");
> +   m264_decoder->release_frame_buffer = dlsym(lib_handle, "m264_ffmpeg_decoder_release_frame_buffer");
> +   m264_decoder->lib_handle = lib_handle;

If it fails to load, you just continue? Honestly?

> +   switch (avctx->field_order)
> +   {
> +      case AV_FIELD_UNKNOWN: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_UNKNOWN \n");
> +         break;
> +      case AV_FIELD_PROGRESSIVE: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_PROGRESSIVE \n");
> +         break;
> +      case AV_FIELD_TT: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TT \n");
> +         break;
> +      case AV_FIELD_BB: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BB \n");
> +         break;
> +      case AV_FIELD_TB: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TB \n");
> +         break;
> +      case AV_FIELD_BT: 
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BT \n");
> +         break;
> +      default:
> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is default \n");
> +         assert(false);
> +         break;
> +   }

Probably much too noisy. And coded way too complicated.

> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->width = %d\n", avctx->width);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->height = %d\n", avctx->height);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);

Probably much too noisy. And even if, could be in just one line.

> +   avctx->pix_fmt = AV_PIX_FMT_YUYV422;

That's all it supports?

> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_decode_close: 2 \n");

"2"?

> +++ b/libavcodec/m264enc.c
[...]
> +#include "m264enc.h"
[...]
> +#include "m264enc.h"

Twice? Please, come on. If even such simple things are done
incorrectly, how do you expect anyone to take the time to review the
rest of the code?

> +#ifdef _WIN32
> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);
> +#else   
> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);
> +#endif

I'm not sure this is acceptable within ffmpeg.

> +   printf("m264_encode_init_h264: avctx->width = %d\n", avctx->width);
> +   printf("m264_encode_init_h264: avctx->height = %d\n", avctx->height);
> +   printf("m264_encode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);
> +   printf("m264_encode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);
> +   printf("m264_encode_init_h264: avctx->gop_size = %d\n", avctx->gop_size);
> +   printf("m264_encode_init_h264: avctx->bit_rate = %" PRId64 "\n", avctx->bit_rate);

This is debug code and does not belong into a released codec.
Furthermore, ffmpeg provides av_log() functions.

> +      {
> +         result = sws_scale(m264_encoder->sw_context, (const uint8_t * const*)frame->data, frame->linesize, 0, frame->height, dst, dst_stride);
> +      }

I'm sure an eventual dependency to sws_scale must be registered in
configure.

> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_encode_close 1");

"1"?


And apart from this superficial review, there are probably quite a lot
of other quality issues.

Moritz

Hi Tomas

Thanks for the review.

My codec is a hardware codec, it depends an dynamic library that goes 
with the M264 card.

The way it works will be:

1>Our customers buy the card and install the drivers of it.

2>They Compile ffmpeg.

3>Call ffmpeg with codec option of m264.

That's why I use dlopen to load the dynamic library that talks with the 
hardware.

Please help us check if it works in ffmpeg.

Thanks.

Yufei.



On 02/25/2019 03:45 PM, Tomas Härdin wrote:
> Hi

>

>> +void convert_to_annexb(unsigned char * dest, unsigned long

>> data_size)

>> +{

>> +   unsigned char *current = dest;

>> +   union

>> +   {

>> +      unsigned char by4[4];

>> +      uint32_t length;

>> +   } union_u32_byte;

>> +

>> +   while ((dest + data_size) > (current + 4))

>> +   {

>> +      if((current[0] == 0) &&

>> +         (current[1] == 0) &&

>> +         (current[2] == 0) &&

>> +         (current[3] == 1))

>> +      {

>> +         // in case it is already in annex B

>> +         break;

>> +      }

>> +

>> +      union_u32_byte.by4[3] = current[0];

>> +      union_u32_byte.by4[2] = current[1];

>> +      union_u32_byte.by4[1] = current[2];

>> +      union_u32_byte.by4[0] = current[3];

> This won't work on certain machines. Type punning is also undefined if

> memory serves me right, and may not work right in all compilers.

> Definitely not recommended. There are macros in FFmpeg for this already

> (AV_RL32 and friends)

>

>> +#ifdef _WIN32

>> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);

>> +#else

>> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);

>> +#endif

> I believe FFmpeg has policy specifically against stuff like this. Link

> the dynamic library properly, and put appropriate license stuff in

> configure. The resulting ffmpeg binaries will more than likely not be

> redistributable, libav* might also be depending on what the Matrox

> library's license is.

>

>> +   printf("m264_encode_init_h264: avctx->width = %d\n", avctx->width);

>> +   printf("m264_encode_init_h264: avctx->height = %d\n", avctx->height);

>> +   printf("m264_encode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);

>> +   printf("m264_encode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);

>> +   printf("m264_encode_init_h264: avctx->gop_size = %d\n", avctx->gop_size);

>> +   printf("m264_encode_init_h264: avctx->bit_rate = %" PRId64 "\n", avctx->bit_rate);

> Forgotten debug stuff? printf() is definitely not OK

>

> There are other issues with this patch, but there's no point in looking

> more for now

>

> /Tomas

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Hi Moritz

It's the first time that I send patch to ffmpeg, for sure I don't know a 
lot of things for ffmpeg.

Thanks a lot for your detailed review.

I'll read the doc and fix the issues you said.

Yufei.


On 02/25/2019 04:21 PM, Moritz Barsnick wrote:
> Hi Yufei,

> before providing large patches here, do read this mailing list and the

> ffmpeg codebase for a while. It will help you to understand the

> process, and to understand how ffmpeg's sources are organized.

>

> I'm sure you missed this:

> https://www.ffmpeg.org/developer.html

>

> Read all of it thoroughly.

>

> The content of this section:

> https://www.ffmpeg.org/developer.html#Coding-Rules-1

> especially comes to mind. Your code uses totally different formatting

> than the rest of the ffmpeg codebase. You should take a look around as

> see how others do it, and what that style guide says.

>

> Apart from that: Everything that Nicolas wrote.

>

> In addition this:

>

> On Mon, Feb 25, 2019 at 19:49:43 +0000, Yufei He wrote:

>> index c90f119..f70368b 100644

>> --- a/Changelog

>> +++ b/Changelog

>> @@ -11,6 +11,7 @@ version <next>:

>>   - dhav demuxer

>>   - PCM-DVD encoder

>>   - GIF parser

>> +- M264 encoder

> Your patch is against an at least two months old version of ffmpeg git.

> Please merge it to latest git HEAD and create a new patch. Your patch

> won't apply currently, and therefore noone will bother to test it.

>

> And even if I try to work around that, here's what happens:

>

> LD      ffmpeg_g

> /usr/bin/ld: libavcodec/libavcodec.a(m264enc.o): in function `ff_m264_encode_init':

> /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:98: undefined reference to `dlopen'

> /usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:108: undefined reference to `dlsym'

> /usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:109: undefined reference to `dlsym'

> /usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:110: undefined reference to `dlsym'

> /usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:111: undefined reference to `dlsym'

> /usr/bin/ld: /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:112: undefined reference to `dlsym'

> /usr/bin/ld: libavcodec/libavcodec.a(m264enc.o):/home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:113: more undefined references to `dlsym' follow

> /usr/bin/ld: libavcodec/libavcodec.a(m264enc.o): in function `ff_m264_encode_close':

> /home/barsnick/Development/ffmpeg-stuff/ffmpeg/libavcodec/m264enc.c:264: undefined reference to `dlclose'

> collect2: error: ld returned 1 exit status

> make: *** [Makefile:108: ffmpeg_g] Error 1

>

> You need to get your dependencies right.

>

>> +   if(*got_output)

>> +   {

>> +      if(decoded_frame->width == 0)

>> +      {

>> +         av_log(NULL, AV_LOG_DEBUG, "Frame parameters mismatch context %d,%d,%d != %d,%d,%d\n",

>> +               decoded_frame->width,

>> +               decoded_frame->height,

>> +               decoded_frame->format,

>> +               ist->dec_ctx->width,

>> +               ist->dec_ctx->height,

>> +               ist->dec_ctx->pix_fmt);

>> +      }

>> +   }

> This is debug code and does not belong into a released codec.

> Furthermore, ffmpeg provides av_log() functions.

>

>> index 0000000..dc1852f

>> --- /dev/null

>> +++ b/libavcodec/.vscode/settings.json

> Don't commit your local development environment's settings, please.

>

>>   OBJS-$(CONFIG_DNXHD_DECODER)           += dnxhddec.o dnxhddata.o

>>   OBJS-$(CONFIG_DNXHD_ENCODER)           += dnxhdenc.o dnxhddata.o

>> +OBJS-$(CONFIG_M264_ENCODER)            += m264enc.o

>> +OBJS-$(CONFIG_M264_DECODER)            += m264dec.o

>>   OBJS-$(CONFIG_DOLBY_E_DECODER)         += dolby_e.o kbdwin.o

>>   OBJS-$(CONFIG_DPX_DECODER)             += dpx.o

> Do you realize that the rest of this list is in alphabetical order?

>

>>   OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF)   += vp9_superframe_split_bsf.o

>>   

>> +

>> +

>> +

>>   # thread libraries

> Why do you add useless empty lines, and commit them?

>

>>   extern AVCodec ff_dnxhd_encoder;

>>   extern AVCodec ff_dnxhd_decoder;

>> +extern AVCodec ff_m264_encoder;

>> +extern AVCodec ff_m264_decoder;

>>   extern AVCodec ff_dpx_encoder;

>>   extern AVCodec ff_dpx_decoder;

> Alphabetical order, again.

>

>>       if (c)

>> -        *opaque = (void*)(i + 1);

>> -

>> +      *opaque = (void*)(i + 1);

>> +

>>       return c;

> Useless change. And please don't ever leave whitespace at your line

> endings, it won't be accepted. (Your IDE surely can fix this for you.)

>

>> +#define FF_PROFILE_M264             0

> What is this?

>

>> +        .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_REORDER,

>> +        .profiles  = NULL_IF_CONFIG_SMALL(ff_h264_profiles),

> Does the encoder even honor any of the profiles?

>

>> +   #ifdef _WIN32

>> +   av_log(avctx, AV_LOG_DEBUG, "_WIN32\n");

>> +   #elif defined _WIN64

>> +   av_log(avctx, AV_LOG_DEBUG, "_WIN64\n");

>> +   #else

>> +   av_log(avctx, AV_LOG_DEBUG, "linux\n");

>> +   #endif

> What for?

>

>> +#ifdef _WIN32

>> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);

>> +#else

>> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);

>> +#endif

> I'm not sure this is acceptable within ffmpeg.

>

>> +   if (!lib_handle)

>> +   {

>> +      av_log(avctx, AV_LOG_ERROR, "failed to load mvM264ffmpeg\n");

>> +   }

>> +

>> +   m264_decoder =  av_mallocz(sizeof(M264Decoder));

>> +

>> +   m264_decoder->init_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_init");

>> +   m264_decoder->exit_m264_decoder = dlsym(lib_handle, "m264_ffmpeg_decoder_exit");

>> +   m264_decoder->send_packet = dlsym(lib_handle, "m264_ffmpeg_decoder_send_packet");

>> +   m264_decoder->receive_frame = dlsym(lib_handle, "m264_ffmpeg_decoder_receive_frame");

>> +   m264_decoder->release_frame_buffer = dlsym(lib_handle, "m264_ffmpeg_decoder_release_frame_buffer");

>> +   m264_decoder->lib_handle = lib_handle;

> If it fails to load, you just continue? Honestly?

>

>> +   switch (avctx->field_order)

>> +   {

>> +      case AV_FIELD_UNKNOWN:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_UNKNOWN \n");

>> +         break;

>> +      case AV_FIELD_PROGRESSIVE:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_PROGRESSIVE \n");

>> +         break;

>> +      case AV_FIELD_TT:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TT \n");

>> +         break;

>> +      case AV_FIELD_BB:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BB \n");

>> +         break;

>> +      case AV_FIELD_TB:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_TB \n");

>> +         break;

>> +      case AV_FIELD_BT:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is AV_FIELD_BT \n");

>> +         break;

>> +      default:

>> +         av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->field_order is default \n");

>> +         assert(false);

>> +         break;

>> +   }

> Probably much too noisy. And coded way too complicated.

>

>> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->width = %d\n", avctx->width);

>> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->height = %d\n", avctx->height);

>> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);

>> +   av_log(avctx, AV_LOG_DEBUG, "m264_decode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);

> Probably much too noisy. And even if, could be in just one line.

>

>> +   avctx->pix_fmt = AV_PIX_FMT_YUYV422;

> That's all it supports?

>

>> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_decode_close: 2 \n");

> "2"?

>

>> +++ b/libavcodec/m264enc.c

> [...]

>> +#include "m264enc.h"

> [...]

>> +#include "m264enc.h"

> Twice? Please, come on. If even such simple things are done

> incorrectly, how do you expect anyone to take the time to review the

> rest of the code?

>

>> +#ifdef _WIN32

>> +   lib_handle = dlopen("mvM264Ffmpeg.dll", RTLD_LAZY);

>> +#else

>> +   lib_handle = dlopen("libmvM264Ffmpeg.so", RTLD_LAZY);

>> +#endif

> I'm not sure this is acceptable within ffmpeg.

>

>> +   printf("m264_encode_init_h264: avctx->width = %d\n", avctx->width);

>> +   printf("m264_encode_init_h264: avctx->height = %d\n", avctx->height);

>> +   printf("m264_encode_init_h264: avctx->framerate.num = %d\n", avctx->framerate.num);

>> +   printf("m264_encode_init_h264: avctx->framerate.den = %d\n", avctx->framerate.den);

>> +   printf("m264_encode_init_h264: avctx->gop_size = %d\n", avctx->gop_size);

>> +   printf("m264_encode_init_h264: avctx->bit_rate = %" PRId64 "\n", avctx->bit_rate);

> This is debug code and does not belong into a released codec.

> Furthermore, ffmpeg provides av_log() functions.

>

>> +      {

>> +         result = sws_scale(m264_encoder->sw_context, (const uint8_t * const*)frame->data, frame->linesize, 0, frame->height, dst, dst_stride);

>> +      }

> I'm sure an eventual dependency to sws_scale must be registered in

> configure.

>

>> +   av_log(avctx, AV_LOG_DEBUG, "ff_m264_encode_close 1");

> "1"?

>

>

> And apart from this superficial review, there are probably quite a lot

> of other quality issues.

>

> Moritz

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Yufei He (12019-02-26):
> My codec is a hardware codec, it depends an dynamic library that goes 
> with the M264 card.
> 
> The way it works will be:
> 
> 1>Our customers buy the card and install the drivers of it.
> 
> 2>They Compile ffmpeg.
> 
> 3>Call ffmpeg with codec option of m264.
> 
> That's why I use dlopen to load the dynamic library that talks with the 
> hardware.

That is not how FFmpeg does things; FFmpeg uses standard linking, unless
there is a very very good reason not to.

> Please help us check if it works in ffmpeg.

If you want to contribute, you need to learn how to interact with this
project. Read the documentation, read the development policy, look how
people who actually contribute act on the mailing list, etc. For
example, look up the thing about top-posting.

Unless you do that, you will not be able to contribute. And it will be
the last time you hear from me.

Regards,

tis 2019-02-26 klockan 14:26 +0000 skrev Yufei He:
> Hi Tomas
> 
> Thanks for the review.
> 
> My codec is a hardware codec, it depends an dynamic library that goes 
> with the M264 card.
> 
> The way it works will be:
> 
> 1>Our customers buy the card and install the drivers of it.
> 
> 2>They Compile ffmpeg.
>
> 3>Call ffmpeg with codec option of m264.
> 
> That's why I use dlopen to load the dynamic library that talks with the 
> hardware.

Nothing in there indicates why dlopen() is a requirement. It is almost
always used for GPL circumvention, so you'd need to provide a very good
reason for why you need to do things via libdl.

I suggest taking a look at the other hwaccel codecs, and the
documentation as Nicolas suggests.

> Please help us check if it works in ffmpeg.

Testing if it works is kind of hard without the hardware.

/Tomas