[FFmpeg-devel] [PATCH v2] Add support for vp9 in iso-bmff

[Ronald S. Bultje]

Hi,

On Mon, Jun 13, 2016 at 5:26 PM, Kongqun Yang <yangkongqun at gmail.com> wrote:
>
> @@ -5369,6 +5384,17 @@ static int mov_write_header(AVFormatContext *s)
>                          pix_fmt == AV_PIX_FMT_MONOWHITE ||
>                          pix_fmt == AV_PIX_FMT_MONOBLACK;
>              }
> +            if (track->mode == MODE_MP4 &&
> +                track->par->codec_id == AV_CODEC_ID_VP9) {
> +              if (s->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
> +                av_log(s, AV_LOG_ERROR,
> +                       "VP9 in MP4 support is experimental, add "
> +                       "'-strict %d' if you want to use it.\n",
> +                       FF_COMPLIANCE_EXPERIMENTAL);
> +                ret = AVERROR_EXPERIMENTAL;
> +                goto error;
> +              }
> +            }
>

Please use 4-space indentation (you're using 2).


> +static int get_vpx_color_space(enum AVColorSpace color_space)
> +{
> +    switch (color_space) {
> +        case AVCOL_SPC_RGB:
> +            return VPX_COLOR_SPACE_RGB;
> +        case AVCOL_SPC_BT709:
> +            return VPX_COLOR_SPACE_BT709;
> +        case AVCOL_SPC_SMPTE170M:
> +            return VPX_COLOR_SPACE_SMPTE_170;
> +        case AVCOL_SPC_SMPTE240M:
> +            return VPX_COLOR_SPACE_SMPTE_240;
> +        case AVCOL_SPC_BT2020_NCL:
> +            return VPX_COLOR_SPACE_BT2020_NCL;
> +        case AVCOL_SPC_BT2020_CL:
> +            return VPX_COLOR_SPACE_BT2020_CL;
> +        default:
> +            return VPX_COLOR_SPACE_UNSPECIFIED;
> +    }
> +}
>
 [..]

> +static int get_vpx_chroma_subsampling(enum AVPixelFormat pixel_format,
> +                                      enum AVChromaLocation
> chroma_location)
> +{
> +    switch (pixel_format) {
> +        case AV_PIX_FMT_YUV420P:
> +        case AV_PIX_FMT_YUV420P10LE:
> +        case AV_PIX_FMT_YUV420P10BE:
> +        case AV_PIX_FMT_YUV420P12LE:
> +        case AV_PIX_FMT_YUV420P12BE:
> +            if (chroma_location == AVCHROMA_LOC_LEFT)
> +                return VPX_SUBSAMPLING_420_VERTICAL;
> +            // Otherwise assume collocated.
> +            return VPX_SUBSAMPLING_420_COLLOCATED_WITH_LUMA;
> +        case AV_PIX_FMT_YUV422P:
> +        case AV_PIX_FMT_YUV422P10LE:
> +        case AV_PIX_FMT_YUV422P10BE:
> +        case AV_PIX_FMT_YUV422P12LE:
> +        case AV_PIX_FMT_YUV422P12BE:
> +            return VPX_SUBSAMPLING_422;
> +        case AV_PIX_FMT_YUV444P:
> +        case AV_PIX_FMT_YUV444P10LE:
> +        case AV_PIX_FMT_YUV444P10BE:
> +        case AV_PIX_FMT_YUV444P12LE:
> +        case AV_PIX_FMT_YUV444P12BE:
> +            return VPX_SUBSAMPLING_444;
> +        default:
> +            av_log(NULL, AV_LOG_ERROR, "Unknown pixel format.");
> +            return -1;
> +    }
> +}
>

In ffmpeg, the case and switch should be indented at the same level:

switch (a) {
case b:
    foo();
    break;
default:
    bar();
    break;
}

What happens if the "default:" case is reached? Is the vpcc atom still
valid? If not, should the muxer error out? Can this even happen at all?

+static int get_vpx_transfer_function(
> +    enum AVColorTransferCharacteristic transfer)
> +{
> +  return (transfer == AVCOL_TRC_SMPTEST2084) ? 1 : 0;
> +}
> +
> +static int get_vpx_video_full_range_flag(enum AVColorRange color_range)
> +{
> +    return (color_range == AVCOL_RANGE_JPEG) ? 1 : 0;
> +}
>

The (..) ? 1 : 0 construct is unnecessary, you can just return .. (or in
case of non-boolean values, we prefer !!(..)).

I also think it's a bad habit to log generic messages like "unknown pixel
format" without a logging context, the user will never understand what that
means.

Ronald