[FFmpeg-devel] [PATCH 02/18] h264: Add stream constraint values to the common header

[Mark Thompson]

On 21/08/17 18:04, Michael Niedermayer wrote:
> On Mon, Aug 21, 2017 at 11:56:11AM +0100, Mark Thompson wrote:
>> On 21/08/17 02:09, Michael Niedermayer wrote:
>>> On Sun, Aug 20, 2017 at 11:41:30PM +0100, Mark Thompson wrote:
>>>> With comments describing the derivation of each value.
>>>>
>>>> (cherry picked from commit aaf441465080b9bc57f5ca8dea656f9b2c5dc821)
>>>> ---
>>>>  libavcodec/h264.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 45 insertions(+)
>>>>
>>>> diff --git a/libavcodec/h264.h b/libavcodec/h264.h
>>>> index 86df5eb9b3..650580bf3a 100644
>>>> --- a/libavcodec/h264.h
>>>> +++ b/libavcodec/h264.h
>>>> @@ -44,4 +44,49 @@ enum {
>>>>      H264_NAL_AUXILIARY_SLICE = 19,
>>>>  };
>>>>  
>>>> +
>>>> +enum {
>>>
>>> why is this a enum ?
>>
>> I believe that compile-time symbolic constants are preferable to preprocessor string replacement.
> 
> I dont mind, but there are many more #defines in the codebase for
> which this argument should apply too.

I agree!  It is nontrivial work with minor gain to change them when they already exist, though.

>>>> +    // 7.4.2.1.1: seq_parameter_set_id is in [0, 31].
>>>> +    H264_MAX_SPS_COUNT = 32,
>>>
>>> Could be doxygen comment compatible
>>
>> Would that have any value?  The comments are explaining the derivation for verification purposes rather than anything to do with the actual use.
> 
> It would put the value into the doxygen on the webpage. Would make
> the comment easy machine associatable to the constant.
> 
> Not sure thats enough "value", I just noticed that you seem to have
> quite exhaustivly documented these constants, it appeared odd that
> if you go to the troubble to do that that they arent doxygen parsable.
> 
> 
>>
>>>> +    // A.3: in table A-1 the highest level allows a MaxFS of 139264.
>>>> +    H264_MAX_MB_PIC_SIZE = 139264,
>>>> +    // A.3.1, A.3.2: PicWidthInMbs and PicHeightInMbs are constrained
>>>> +    // to be not greater than sqrt(MaxFS * 8).  Hence height/width are
>>>> +    // bounded above by sqrt(139264 * 8) = 1055.5 macroblocks.
>>>> +    H264_MAX_MB_WIDTH    = 1055,
>>>> +    H264_MAX_MB_HEIGHT   = 1055,
>>>> +    H264_MAX_WIDTH       = H264_MAX_MB_WIDTH  * 16,
>>>> +    H264_MAX_HEIGHT      = H264_MAX_MB_HEIGHT * 16,
>>>
>>> There should be no problem with files outside these limits. They would
>>> be valid H.264 files, just not within the level and profile constraints
>>> of the currently defined profiles and levels.
>>> or do i miss something ?
>> Currently these values are used for some fixed-size tables in cbs (e.g. H264_MAX_MB_PIC_SIZE is needed for slice_group_id[]).  More generally, I don't want to consider files which don't conform to some level limits - once you allow MaxDPBFrames to exceed 16 the world is a terrible place.
>>
> 
>> (I am not intending to add this constraint to the existing decoder.)
> 
> but some of the newly added code uses the resolution limits IIRC.
> 
> We never previosuly limited resolution based on profile/level.
> Can you summarize what features would be restricted to 1055 MBs
> resolution ?

All bitstream filters based on the cbs infrastructure, and also anything else using it (VAAPI encode, but that will have hardware restrictions which are smaller anyway).

They will also be restricted by the other level limits in this file - max_num_ref_frames at 16 (also enforced by the decoder) and num_slice_groups_minus1 at 7 (nonzero values not supported by the decoder at all).

The intent of cbs here is very much to be able to fully parse all standard streams, and to immediately reject anything nonstandard.  If we want some sort of nonstandard support later then it could be added separately, suitably gated by options, but I don't want to do that now.

- Mark