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

KongQun Yang kqyang at google.com
Wed Jun 15 00:02:38 CEST 2016


Thanks for the review.  Please see patchset v4 for the new change. (Please
ignore patchset v3 which is uploaded incorrectly)

-- KongQun Yang (KQ)

On Tue, Jun 14, 2016 at 1:11 PM, Ronald S. Bultje <rsbultje at gmail.com>
wrote:

> 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).
>

Good catch. Done.

>
>
>> +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;
> }
>

Done.

>
> 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?
>

I assume you were talking about the color space. The spec allows
unspecified color space (with value 0), so the vpcc atom is valid in this
case. It does happen as the default color space is AVCOL_SPC_UNSPECIFIED.
I have updated the code to make it explicit: vpx color space will be set to
unspecified only if |par->color_space == AVCOL_SPC_UNSPECIFIED|. The code
will error out if an unrecognized color space is encountered.

>
> +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 !!(..)).
>
Done.

>
> 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.
>
Done (included the pixel format code in the message).

>
> Ronald
>


More information about the ffmpeg-devel mailing list