A few comments below.
On 12/12/2018 16:26, Guo, Yejun wrote:
> + if (frame->rois_buf != NULL) {
> + if (x4->params.rc.i_aq_mode == X264_AQ_NONE) {
> + av_log(ctx, AV_LOG_ERROR, "Adaptive quantization must be
> enabled to use ROI encoding, skipping ROI.\n");
This should, in my opinion, return an error and fail hard.
If people want it to continue anyway, it should be AV_LOG_WARNING.
> + } else {
> + if (frame->interlaced_frame == 0) {
> + const static int MBSIZE = 16;
I think we generally use defines for this stuff.
> + size_t mbx = (frame->width + MBSIZE - 1) / MBSIZE;
> + size_t mby = (frame->height + MBSIZE - 1) / MBSIZE;
> + float* qoffsets = (float*)av_malloc(sizeof(float) * mbx
> * mby);
Convention in FFmpeg is to use sizeof(*var).
> + memset(qoffsets, 0, sizeof(float) * mbx * mby);
Missing NULL check for alloc failure.
> +
> + size_t nb_rois = frame->rois_buf->size /
> sizeof(AVFrameROI);
I think we have some macros that do this already.
> + AVFrameROI* rois = (AVFrameROI*)frame->rois_buf->data;
> + for (size_t roi = 0; roi < nb_rois; ++roi) {
Nit/convention: roi++
> + int starty = FFMIN(mby, rois[roi].top / MBSIZE);
> + int endy = FFMIN(mby, (rois[roi].bottom + MBSIZE -
> 1)/ MBSIZE);
> + int startx = FFMIN(mbx, rois[roi].left / MBSIZE);
> + int endx = FFMIN(mbx, (rois[roi].right + MBSIZE -
> 1)/ MBSIZE);
> + for (int y = starty; y < endy; ++y) {
> + for (int x = startx; x < endx; ++x) {
> + qoffsets[x + y*mbx] = get_roi_qoffset(ctx,
> rois[roi].quality);
> + }
> + }
> + }
> +
> + x4->pic.prop.quant_offsets = qoffsets;
> + x4->pic.prop.quant_offsets_free = av_free;
> + } else {
> + av_log(ctx, AV_LOG_ERROR, "interlaced_frame not
> supported for ROI encoding yet, skipping ROI.\n");
Same comment as befor: return error or change to warning.
> +enum AVRoiQuality {
Probably should be AVROIQuality.
> + AV_RQ_NONE = 0,
> + AV_RQ_BETTER = 1,
> + AV_RQ_BEST = 2,
> +};
> +
> +typedef struct AVFrameROI {
> + /* coordinates at frame pixel level.
> + * it will be extended internally if the codec requirs an alignment
> + */
> + size_t top;
> + size_t bottom;
> + size_t left;
> + size_t right;
> + enum AVRoiQuality quality;
> +} AVFrameROI;
Are we not going to allow the API to set an actual offset? This really
limits what someone can do. (I say this as a user of x264's ROI API, in my
own codebase, at least.)
Cheers,
- Derek
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
