diff mbox series

[FFmpeg-devel,3/7] avcodec/golomb, h264*: Fix get_ue_golomb_31()

Message ID 20200714153454.9354-2-andreas.rheinhardt@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avcodec/golomb: Prevent shift by negative number | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt July 14, 2020, 3:34 p.m. UTC 
get_ue_golomb_31() uses a LUT of 512 entries; therefore it can be used
to parse exp-golomb codes of length <= 9, i.e. those codes with at most
four leading bits that have five effective bits; this implies a range of
0..30 and not 31. In particular, this function must not be used to parse
e.g. the H.264 SPS id.

This commit renames the function to get_ue_golomb_30() and uses it
whereever possible. In places where it is not possible, it has been
replaced by get_ue_golomb2(). Given that the returned value of said
function can be a negative error code, it is no longer included in
av_log() statements (the parsed value can actually also be wrong when
using get_ue_golomb30()).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
I have only used get_ue_golomb_30() where I am certain that its range is
sufficient; this unfortunately excludes certain cases in h264_cavlc,
because for field based content, the number of refs can be in the range
of 0..31.

 libavcodec/golomb.h      |  6 +++---
 libavcodec/h264_cavlc.c  | 14 +++++++-------
 libavcodec/h264_parser.c |  8 ++++----
 libavcodec/h264_ps.c     | 18 +++++++++---------
 libavcodec/h264_refs.c   |  6 +++---
 libavcodec/h264_sei.c    |  8 ++++----
 libavcodec/h264_slice.c  |  6 +++---
 libavformat/mxfenc.c     |  2 +-
 8 files changed, 34 insertions(+), 34 deletions(-)

Comments

Michael Niedermayer July 14, 2020, 8:36 p.m. UTC | #1 
On Tue, Jul 14, 2020 at 05:34:50PM +0200, Andreas Rheinhardt wrote:
> get_ue_golomb_31() uses a LUT of 512 entries; therefore it can be used
> to parse exp-golomb codes of length <= 9, i.e. those codes with at most
> four leading bits that have five effective bits; this implies a range of
> 0..30 and not 31. In particular, this function must not be used to parse
> e.g. the H.264 SPS id.

hmm, are you sure ?

1           0
01X         1-2
001XX       3-6
0001XXX     7-14
00001XXXX   15-30
000001..... 31

we need to read 9 bits for this, we do not need to read the bits marked with
a "." because the code is already determined at this point.



[...]
Andreas Rheinhardt July 14, 2020, 8:43 p.m. UTC | #2 
Michael Niedermayer:
> On Tue, Jul 14, 2020 at 05:34:50PM +0200, Andreas Rheinhardt wrote:
>> get_ue_golomb_31() uses a LUT of 512 entries; therefore it can be used
>> to parse exp-golomb codes of length <= 9, i.e. those codes with at most
>> four leading bits that have five effective bits; this implies a range of
>> 0..30 and not 31. In particular, this function must not be used to parse
>> e.g. the H.264 SPS id.
> 
> hmm, are you sure ?
> 

Yes.

> 1           0
> 01X         1-2
> 001XX       3-6
> 0001XXX     7-14
> 00001XXXX   15-30
> 000001..... 31
> 
> we need to read 9 bits for this, we do not need to read the bits marked with
> a "." because the code is already determined at this point.
> 
The code is only determined at the point if one already presumes that it
can't be anything >31. Without this assumption, one needs to look at all
the five "." in order to distinguish it from 32..62. By looking at only
9 digits one can not rule out 32-34.
And if you take a look at ff_ue_golomb_vlc_code, you will see that it
does not even contain 31 at all. It returns the values 0-30 for the
codes with length <= 9 and returns 32 for the rest.

- Andreas
Michael Niedermayer July 14, 2020, 9:01 p.m. UTC | #3 
On Tue, Jul 14, 2020 at 10:43:51PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Tue, Jul 14, 2020 at 05:34:50PM +0200, Andreas Rheinhardt wrote:
> >> get_ue_golomb_31() uses a LUT of 512 entries; therefore it can be used
> >> to parse exp-golomb codes of length <= 9, i.e. those codes with at most
> >> four leading bits that have five effective bits; this implies a range of
> >> 0..30 and not 31. In particular, this function must not be used to parse
> >> e.g. the H.264 SPS id.
> > 
> > hmm, are you sure ?
> > 
> 
> Yes.
> 
> > 1           0
> > 01X         1-2
> > 001XX       3-6
> > 0001XXX     7-14
> > 00001XXXX   15-30
> > 000001..... 31
> > 
> > we need to read 9 bits for this, we do not need to read the bits marked with
> > a "." because the code is already determined at this point.
> > 
> The code is only determined at the point if one already presumes that it
> can't be anything >31. 

yes, that is the idea of get_ue_golomb_31() its only used when the biggest
valid code is 31.


> Without this assumption, one needs to look at all
> the five "." in order to distinguish it from 32..62. By looking at only
> 9 digits one can not rule out 32-34.
> And if you take a look at ff_ue_golomb_vlc_code, you will see that it
> does not even contain 31 at all. It returns the values 0-30 for the
> codes with length <= 9 and returns 32 for the rest.

const uint8_t ff_ue_golomb_vlc_code[512]={
32,32,32,32,32,32,32,32,31,32,32,32,32,32,32,32,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,
                        ^^
[...]
Andreas Rheinhardt July 14, 2020, 9:14 p.m. UTC | #4 
Michael Niedermayer:
> On Tue, Jul 14, 2020 at 10:43:51PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Tue, Jul 14, 2020 at 05:34:50PM +0200, Andreas Rheinhardt wrote:
>>>> get_ue_golomb_31() uses a LUT of 512 entries; therefore it can be used
>>>> to parse exp-golomb codes of length <= 9, i.e. those codes with at most
>>>> four leading bits that have five effective bits; this implies a range of
>>>> 0..30 and not 31. In particular, this function must not be used to parse
>>>> e.g. the H.264 SPS id.
>>>
>>> hmm, are you sure ?
>>>
>>
>> Yes.
>>
>>> 1           0
>>> 01X         1-2
>>> 001XX       3-6
>>> 0001XXX     7-14
>>> 00001XXXX   15-30
>>> 000001..... 31
>>>
>>> we need to read 9 bits for this, we do not need to read the bits marked with
>>> a "." because the code is already determined at this point.
>>>
>> The code is only determined at the point if one already presumes that it
>> can't be anything >31. 
> 
> yes, that is the idea of get_ue_golomb_31() its only used when the biggest
> valid code is 31.
> 
But then we still have to rely on the code being valid as we have no way
to distinguish 31 from 32-34. Is this considered acceptable for the
speed gain?

> 
>> Without this assumption, one needs to look at all
>> the five "." in order to distinguish it from 32..62. By looking at only
>> 9 digits one can not rule out 32-34.
>> And if you take a look at ff_ue_golomb_vlc_code, you will see that it
>> does not even contain 31 at all. It returns the values 0-30 for the
>> codes with length <= 9 and returns 32 for the rest.
> 
> const uint8_t ff_ue_golomb_vlc_code[512]={
> 32,32,32,32,32,32,32,32,31,32,32,32,32,32,32,32,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,
>                         ^^

Indeed, how could I overlook this. Must have been blind. Sorry for that.

- Andreas
Michael Niedermayer July 15, 2020, 6:38 a.m. UTC | #5 
On Tue, Jul 14, 2020 at 11:14:19PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Tue, Jul 14, 2020 at 10:43:51PM +0200, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >>> On Tue, Jul 14, 2020 at 05:34:50PM +0200, Andreas Rheinhardt wrote:
> >>>> get_ue_golomb_31() uses a LUT of 512 entries; therefore it can be used
> >>>> to parse exp-golomb codes of length <= 9, i.e. those codes with at most
> >>>> four leading bits that have five effective bits; this implies a range of
> >>>> 0..30 and not 31. In particular, this function must not be used to parse
> >>>> e.g. the H.264 SPS id.
> >>>
> >>> hmm, are you sure ?
> >>>
> >>
> >> Yes.
> >>
> >>> 1           0
> >>> 01X         1-2
> >>> 001XX       3-6
> >>> 0001XXX     7-14
> >>> 00001XXXX   15-30
> >>> 000001..... 31
> >>>
> >>> we need to read 9 bits for this, we do not need to read the bits marked with
> >>> a "." because the code is already determined at this point.
> >>>
> >> The code is only determined at the point if one already presumes that it
> >> can't be anything >31. 
> > 
> > yes, that is the idea of get_ue_golomb_31() its only used when the biggest
> > valid code is 31.
> > 
> But then we still have to rely on the code being valid as we have no way
> to distinguish 31 from 32-34. Is this considered acceptable for the
> speed gain?

Is it a problem when 32-34 (which are invalid) are mapped to 31 ?

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 63069f63e5..172d9934e0 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -134,10 +134,10 @@  static inline unsigned get_ue_golomb_long(GetBitContext *gb)
 }
 
 /**
- * read unsigned exp golomb code, constraint to a max of 31.
- * the return value is undefined if the stored value exceeds 31.
+ * Read unsigned exp golomb code, constraint to a max of 30.
+ * the return value is undefined if the stored value exceeds 30.
  */
-static inline int get_ue_golomb_31(GetBitContext *gb)
+static inline int get_ue_golomb_30(GetBitContext *gb)
 {
     unsigned int buf;
 
diff --git a/libavcodec/h264_cavlc.c b/libavcodec/h264_cavlc.c
index 6481992e58..51091abbe1 100644
--- a/libavcodec/h264_cavlc.c
+++ b/libavcodec/h264_cavlc.c
@@ -838,7 +838,7 @@  decode_intra_mb:
         }
         if(decode_chroma){
             pred_mode= ff_h264_check_intra_pred_mode(h->avctx, sl->top_samples_available,
-                                                     sl->left_samples_available, get_ue_golomb_31(&sl->gb), 1);
+                                                     sl->left_samples_available, get_ue_golomb_30(&sl->gb), 1);
             if(pred_mode < 0)
                 return -1;
             sl->chroma_pred_mode = pred_mode;
@@ -850,7 +850,7 @@  decode_intra_mb:
 
         if (sl->slice_type_nos == AV_PICTURE_TYPE_B) {
             for(i=0; i<4; i++){
-                sl->sub_mb_type[i]= get_ue_golomb_31(&sl->gb);
+                sl->sub_mb_type[i] = get_ue_golomb_30(&sl->gb);
                 if(sl->sub_mb_type[i] >=13){
                     av_log(h->avctx, AV_LOG_ERROR, "B sub_mb_type %u out of range at %d %d\n", sl->sub_mb_type[i], sl->mb_x, sl->mb_y);
                     return -1;
@@ -868,7 +868,7 @@  decode_intra_mb:
         }else{
             av_assert2(sl->slice_type_nos == AV_PICTURE_TYPE_P); //FIXME SP correct ?
             for(i=0; i<4; i++){
-                sl->sub_mb_type[i]= get_ue_golomb_31(&sl->gb);
+                sl->sub_mb_type[i] = get_ue_golomb_30(&sl->gb);
                 if(sl->sub_mb_type[i] >=4){
                     av_log(h->avctx, AV_LOG_ERROR, "P sub_mb_type %u out of range at %d %d\n", sl->sub_mb_type[i], sl->mb_x, sl->mb_y);
                     return -1;
@@ -889,7 +889,7 @@  decode_intra_mb:
                     }else if(ref_count == 2){
                         tmp= get_bits1(&sl->gb)^1;
                     }else{
-                        tmp= get_ue_golomb_31(&sl->gb);
+                        tmp = get_ue_golomb2(&sl->gb);
                         if(tmp>=ref_count){
                             av_log(h->avctx, AV_LOG_ERROR, "ref %u overflow\n", tmp);
                             return -1;
@@ -965,7 +965,7 @@  decode_intra_mb:
                         } else if (rc == 2) {
                             val= get_bits1(&sl->gb)^1;
                         }else{
-                            val= get_ue_golomb_31(&sl->gb);
+                            val = get_ue_golomb2(&sl->gb);
                             if (val >= rc) {
                                 av_log(h->avctx, AV_LOG_ERROR, "ref %u overflow\n", val);
                                 return -1;
@@ -996,7 +996,7 @@  decode_intra_mb:
                             } else if (rc == 2) {
                                 val= get_bits1(&sl->gb)^1;
                             }else{
-                                val= get_ue_golomb_31(&sl->gb);
+                                val = get_ue_golomb2(&sl->gb);
                                 if (val >= rc) {
                                     av_log(h->avctx, AV_LOG_ERROR, "ref %u overflow\n", val);
                                     return -1;
@@ -1034,7 +1034,7 @@  decode_intra_mb:
                             } else if (rc == 2) {
                                 val= get_bits1(&sl->gb)^1;
                             }else{
-                                val= get_ue_golomb_31(&sl->gb);
+                                val = get_ue_golomb2(&sl->gb);
                                 if (val >= rc) {
                                     av_log(h->avctx, AV_LOG_ERROR, "ref %u overflow\n", val);
                                     return -1;
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index aacd44cf3b..8af094075f 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -180,7 +180,7 @@  static int scan_mmco_reset(AVCodecParserContext *s, GetBitContext *gb,
             if (get_bits1(gb)) {
                 int index;
                 for (index = 0; ; index++) {
-                    unsigned int reordering_of_pic_nums_idc = get_ue_golomb_31(gb);
+                    unsigned reordering_of_pic_nums_idc = get_ue_golomb_30(gb);
 
                     if (reordering_of_pic_nums_idc < 3)
                         get_ue_golomb_long(gb);
@@ -210,7 +210,7 @@  static int scan_mmco_reset(AVCodecParserContext *s, GetBitContext *gb,
     if (get_bits1(gb)) { // adaptive_ref_pic_marking_mode_flag
         int i;
         for (i = 0; i < MAX_MMCO_COUNT; i++) {
-            MMCOOpcode opcode = get_ue_golomb_31(gb);
+            MMCOOpcode opcode = get_ue_golomb_30(gb);
             if (opcode > (unsigned) MMCO_LONG) {
                 av_log(logctx, AV_LOG_ERROR,
                        "illegal memory management control operation %d\n",
@@ -226,7 +226,7 @@  static int scan_mmco_reset(AVCodecParserContext *s, GetBitContext *gb,
                 get_ue_golomb_long(gb); // difference_of_pic_nums_minus1
             if (opcode == MMCO_SHORT2LONG || opcode == MMCO_LONG2UNUSED ||
                 opcode == MMCO_LONG || opcode == MMCO_SET_MAX_LONG)
-                get_ue_golomb_31(gb);
+                get_ue_golomb2(gb);
         }
     }
 
@@ -342,7 +342,7 @@  static inline int parse_nal_units(AVCodecParserContext *s,
         /* fall through */
         case H264_NAL_SLICE:
             get_ue_golomb_long(&nal.gb);  // skip first_mb_in_slice
-            slice_type   = get_ue_golomb_31(&nal.gb);
+            slice_type   = get_ue_golomb_30(&nal.gb);
             s->pict_type = ff_h264_golomb_to_pict_type[slice_type % 5];
             if (p->sei.recovery_point.recovery_frame_cnt >= 0) {
                 /* key frame, since recovery_frame_cnt is set */
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index e774929e21..13823b8dc0 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -108,10 +108,10 @@  static inline int decode_hrd_parameters(GetBitContext *gb, void *logctx,
                                         SPS *sps)
 {
     int cpb_count, i;
-    cpb_count = get_ue_golomb_31(gb) + 1;
+    cpb_count = get_ue_golomb2(gb);
 
-    if (cpb_count > 32U) {
-        av_log(logctx, AV_LOG_ERROR, "cpb_count %d invalid\n", cpb_count);
+    if (cpb_count++ > 31U) {
+        av_log(logctx, AV_LOG_ERROR, "cpb_count invalid\n");
         return AVERROR_INVALIDDATA;
     }
 
@@ -361,10 +361,10 @@  int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
     constraint_set_flags |= get_bits1(gb) << 5;   // constraint_set5_flag
     skip_bits(gb, 2);                             // reserved_zero_2bits
     level_idc = get_bits(gb, 8);
-    sps_id    = get_ue_golomb_31(gb);
+    sps_id    = get_ue_golomb2(gb);
 
     if (sps_id >= MAX_SPS_COUNT) {
-        av_log(avctx, AV_LOG_ERROR, "sps_id %u out of range\n", sps_id);
+        av_log(avctx, AV_LOG_ERROR, "sps_id out of range\n");
         goto fail;
     }
 
@@ -391,7 +391,7 @@  int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
         sps->profile_idc == 128 ||  // Multiview High profile (MVC)
         sps->profile_idc == 138 ||  // Multiview Depth High profile (MVCD)
         sps->profile_idc == 144) {  // old High444 profile
-        sps->chroma_format_idc = get_ue_golomb_31(gb);
+        sps->chroma_format_idc = get_ue_golomb_30(gb);
         if (sps->chroma_format_idc > 3U) {
             avpriv_request_sample(avctx, "chroma_format_idc %u",
                                   sps->chroma_format_idc);
@@ -438,7 +438,7 @@  int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
     }
     sps->log2_max_frame_num = log2_max_frame_num_minus4 + 4;
 
-    sps->poc_type = get_ue_golomb_31(gb);
+    sps->poc_type = get_ue_golomb_30(gb);
 
     if (sps->poc_type == 0) { // FIXME #define
         unsigned t = get_ue_golomb(gb);
@@ -482,7 +482,7 @@  int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
         goto fail;
     }
 
-    sps->ref_frame_count = get_ue_golomb_31(gb);
+    sps->ref_frame_count = get_ue_golomb_30(gb);
     if (avctx->codec_tag == MKTAG('S', 'M', 'V', '2'))
         sps->ref_frame_count = FFMAX(2, sps->ref_frame_count);
     if (sps->ref_frame_count > MAX_DELAYED_PIC_COUNT) {
@@ -781,7 +781,7 @@  int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct
     }
     memcpy(pps->data, gb->buffer, pps->data_size);
 
-    pps->sps_id = get_ue_golomb_31(gb);
+    pps->sps_id = get_ue_golomb2(gb);
     if ((unsigned)pps->sps_id >= MAX_SPS_COUNT ||
         !ps->sps_list[pps->sps_id]) {
         av_log(avctx, AV_LOG_ERROR, "sps_id %u out of range\n", pps->sps_id);
diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
index dae8bd278a..4ceaaa3863 100644
--- a/libavcodec/h264_refs.c
+++ b/libavcodec/h264_refs.c
@@ -432,7 +432,7 @@  int ff_h264_decode_ref_pic_list_reordering(H264SliceContext *sl, void *logctx)
             continue;
 
         for (index = 0; ; index++) {
-            unsigned int op = get_ue_golomb_31(&sl->gb);
+            unsigned int op = get_ue_golomb_30(&sl->gb);
 
             if (op == 3)
                 break;
@@ -850,7 +850,7 @@  int ff_h264_decode_ref_pic_marking(H264SliceContext *sl, GetBitContext *gb,
         sl->explicit_ref_marking = get_bits1(gb);
         if (sl->explicit_ref_marking) {
             for (i = 0; i < MAX_MMCO_COUNT; i++) {
-                MMCOOpcode opcode = get_ue_golomb_31(gb);
+                MMCOOpcode opcode = get_ue_golomb_30(gb);
 
                 mmco[i].opcode = opcode;
                 if (opcode == MMCO_SHORT2UNUSED || opcode == MMCO_SHORT2LONG) {
@@ -860,7 +860,7 @@  int ff_h264_decode_ref_pic_marking(H264SliceContext *sl, GetBitContext *gb,
                 }
                 if (opcode == MMCO_SHORT2LONG || opcode == MMCO_LONG2UNUSED ||
                     opcode == MMCO_LONG || opcode == MMCO_SET_MAX_LONG) {
-                    unsigned int long_arg = get_ue_golomb_31(gb);
+                    unsigned int long_arg = get_ue_golomb2(gb);
                     if (long_arg >= 32 ||
                         (long_arg >= 16 && !(opcode == MMCO_SET_MAX_LONG &&
                                              long_arg == 16) &&
diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
index 7b8e6bd7ba..63d43b2bc5 100644
--- a/libavcodec/h264_sei.c
+++ b/libavcodec/h264_sei.c
@@ -320,11 +320,11 @@  static int decode_buffering_period(H264SEIBufferingPeriod *h, GetBitContext *gb,
     int sched_sel_idx;
     const SPS *sps;
 
-    sps_id = get_ue_golomb_31(gb);
-    if (sps_id > 31 || !ps->sps_list[sps_id]) {
+    sps_id = get_ue_golomb2(gb);
+    if (sps_id > 31U || !ps->sps_list[sps_id]) {
         av_log(logctx, AV_LOG_ERROR,
-               "non-existing SPS %d referenced in buffering period\n", sps_id);
-        return sps_id > 31 ? AVERROR_INVALIDDATA : AVERROR_PS_NOT_FOUND;
+               "non-existing SPS referenced in buffering period\n");
+        return sps_id > 31U ? AVERROR_INVALIDDATA : AVERROR_PS_NOT_FOUND;
     }
     sps = (const SPS*)ps->sps_list[sps_id]->data;
 
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index c7b2764270..8f4b7ef1ec 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1739,7 +1739,7 @@  static int h264_slice_header_parse(const H264Context *h, H264SliceContext *sl,
 
     sl->first_mb_addr = get_ue_golomb_long(&sl->gb);
 
-    slice_type = get_ue_golomb_31(&sl->gb);
+    slice_type = get_ue_golomb_30(&sl->gb);
     if (slice_type > 9) {
         av_log(h->avctx, AV_LOG_ERROR,
                "slice type %d too large at %d\n",
@@ -1874,7 +1874,7 @@  static int h264_slice_header_parse(const H264Context *h, H264SliceContext *sl,
     }
 
     if (sl->slice_type_nos != AV_PICTURE_TYPE_I && pps->cabac) {
-        tmp = get_ue_golomb_31(&sl->gb);
+        tmp = get_ue_golomb_30(&sl->gb);
         if (tmp > 2) {
             av_log(h->avctx, AV_LOG_ERROR, "cabac_init_idc %u overflow\n", tmp);
             return AVERROR_INVALIDDATA;
@@ -1902,7 +1902,7 @@  static int h264_slice_header_parse(const H264Context *h, H264SliceContext *sl,
     sl->slice_alpha_c0_offset = 0;
     sl->slice_beta_offset     = 0;
     if (pps->deblocking_filter_parameters_present) {
-        tmp = get_ue_golomb_31(&sl->gb);
+        tmp = get_ue_golomb_30(&sl->gb);
         if (tmp > 2) {
             av_log(h->avctx, AV_LOG_ERROR,
                    "deblocking_filter_idc %u out of range\n", tmp);
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 5a3a609bf6..c7ceb40b55 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2223,7 +2223,7 @@  static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
         case H264_NAL_SLICE:
             init_get_bits8(&gb, buf, buf_end - buf);
             get_ue_golomb_long(&gb); // skip first_mb_in_slice
-            slice_type = get_ue_golomb_31(&gb);
+            slice_type = get_ue_golomb_30(&gb);
             switch (slice_type % 5) {
             case 0:
                 e->flags |= 0x20; // P Picture