[FFmpeg-devel] patch for a new H.264 codec
Yufei He
yhe at matrox.com
Tue Feb 26 16:29:41 EET 2019
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list