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

[Michael Niedermayer]

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.


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170821/0e5e5f4b/attachment.sig>