| Message ID | 20170820224146.21289-3-sw@jkqxz.net |
|---|---|
| State | Accepted |
| Commit | aaf441465080b9bc57f5ca8dea656f9b2c5dc821 |
| Headers | show |
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 ? > + // 7.4.2.1.1: seq_parameter_set_id is in [0, 31]. > + H264_MAX_SPS_COUNT = 32, Could be doxygen comment compatible [...] > + // 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 ? [...]
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. >> + // 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. >> + // 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.) Thanks, - Mark
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 ? [...]
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
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 { + // 7.4.2.1.1: seq_parameter_set_id is in [0, 31]. + H264_MAX_SPS_COUNT = 32, + // 7.4.2.2: pic_parameter_set_id is in [0, 255]. + H264_MAX_PPS_COUNT = 256, + + // A.3: MaxDpbFrames is bounded above by 16. + H264_MAX_DPB_FRAMES = 16, + // 7.4.2.1.1: max_num_ref_frames is in [0, MaxDpbFrames], and + // each reference frame can have two fields. + H264_MAX_REFS = 2 * H264_MAX_DPB_FRAMES, + + // 7.4.3.1: modification_of_pic_nums_idc is not equal to 3 at most + // num_ref_idx_lN_active_minus1 + 1 times (that is, once for each + // possible reference), then equal to 3 once. + H264_MAX_RPLM_COUNT = H264_MAX_REFS + 1, + + // 7.4.3.3: in the worst case, we begin with a full short-term + // reference picture list. Each picture in turn is moved to the + // long-term list (type 3) and then discarded from there (type 2). + // Then, we set the length of the long-term list (type 4), mark + // the current picture as long-term (type 6) and terminate the + // process (type 0). + H264_MAX_MMCO_COUNT = H264_MAX_REFS * 2 + 3, + + // A.2.1, A.2.3: profiles supporting FMO constrain + // num_slice_groups_minus1 to be in [0, 7]. + H264_MAX_SLICE_GROUPS = 8, + + // E.2.2: cpb_cnt_minus1 is in [0, 31]. + H264_MAX_CPB_CNT = 32, + + // 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, +}; + + #endif /* AVCODEC_H264_H */