diff mbox series

[FFmpeg-devel,6/6] lavc/h264_ps: always include the stop bit in [s|p]ps->data

Message ID bf15fa33c739b0a4b4d32bedeec6f326e3b175e0.1652120184.git.ffmpegagent@gmail.com
State New
Headers show
Series lavc/videotoolboxdec: improve HEVC stream compatibility | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

softworkz May 9, 2022, 6:16 p.m. UTC
From: rcombs <rcombs@rcombs.me>

The VideoToolbox hwaccel needs the entire NAL (including the stop bit),
but ff_h2645_packet_split may remove it. Detect this case by looking for
bit counts divisible by 8 and insert a stop-bit-only 0x80 byte.

Signed-off-by: rcombs <rcombs@rcombs.me>
---
 libavcodec/h264_ps.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Andreas Rheinhardt May 10, 2022, 4:26 a.m. UTC | #1
rcombs:
> From: rcombs <rcombs@rcombs.me>
> 
> The VideoToolbox hwaccel needs the entire NAL (including the stop bit),
> but ff_h2645_packet_split may remove it. Detect this case by looking for
> bit counts divisible by 8 and insert a stop-bit-only 0x80 byte.
> 
> Signed-off-by: rcombs <rcombs@rcombs.me>
> ---
>  libavcodec/h264_ps.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
> index 051f06692c..e16da68dec 100644
> --- a/libavcodec/h264_ps.c
> +++ b/libavcodec/h264_ps.c
> @@ -351,6 +351,10 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
>      }
>      memcpy(sps->data, gb->buffer, sps->data_size);
>  
> +    // Re-add the removed stop bit (may be used by hwaccels).
> +    if (!(gb->size_in_bits & 7) && sps->data_size < sizeof(sps->data))
> +        sps->data[sps->data_size++] = 0x80;
> +
>      profile_idc           = get_bits(gb, 8);
>      constraint_set_flags |= get_bits1(gb) << 0;   // constraint_set0_flag
>      constraint_set_flags |= get_bits1(gb) << 1;   // constraint_set1_flag
> @@ -775,6 +779,10 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct
>      }
>      memcpy(pps->data, gb->buffer, pps->data_size);
>  
> +    // Re-add the removed stop bit (may be used by hwaccels).
> +    if (!(bit_length & 7) && pps->data_size < sizeof(pps->data))
> +        pps->data[pps->data_size++] = 0x80;
> +
>      pps->sps_id = get_ue_golomb_31(gb);
>      if ((unsigned)pps->sps_id >= MAX_SPS_COUNT ||
>          !ps->sps_list[pps->sps_id]) {

Modifying these functions affects more than just VideoToolbox. Are you
sure that the other users of this file are ok with this change?

- Andreas
rcombs May 24, 2022, 10:20 p.m. UTC | #2
> On May 9, 2022, at 23:26, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> rcombs:
>> From: rcombs <rcombs@rcombs.me>
>> 
>> The VideoToolbox hwaccel needs the entire NAL (including the stop bit),
>> but ff_h2645_packet_split may remove it. Detect this case by looking for
>> bit counts divisible by 8 and insert a stop-bit-only 0x80 byte.
>> 
>> Signed-off-by: rcombs <rcombs@rcombs.me>
>> ---
>> libavcodec/h264_ps.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>> 
>> diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
>> index 051f06692c..e16da68dec 100644
>> --- a/libavcodec/h264_ps.c
>> +++ b/libavcodec/h264_ps.c
>> @@ -351,6 +351,10 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
>> }
>> memcpy(sps->data, gb->buffer, sps->data_size);
>> 
>> + // Re-add the removed stop bit (may be used by hwaccels).
>> + if (!(gb->size_in_bits & 7) && sps->data_size < sizeof(sps->data))
>> + sps->data[sps->data_size++] = 0x80;
>> +
>> profile_idc = get_bits(gb, 8);
>> constraint_set_flags |= get_bits1(gb) << 0; // constraint_set0_flag
>> constraint_set_flags |= get_bits1(gb) << 1; // constraint_set1_flag
>> @@ -775,6 +779,10 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct
>> }
>> memcpy(pps->data, gb->buffer, pps->data_size);
>> 
>> + // Re-add the removed stop bit (may be used by hwaccels).
>> + if (!(bit_length & 7) && pps->data_size < sizeof(pps->data))
>> + pps->data[pps->data_size++] = 0x80;
>> +
>> pps->sps_id = get_ue_golomb_31(gb);
>> if ((unsigned)pps->sps_id >= MAX_SPS_COUNT ||
>> !ps->sps_list[pps->sps_id]) {
> 
> Modifying these functions affects more than just VideoToolbox. Are you
> sure that the other users of this file are ok with this change?
> 
> - Andreas

It passes FATE, and only adds a byte containing the stop bit (which would be present prior to this commit in 7 of 8 possible cases), so I believe it should be fine (and conceptually including the stop bit makes more sense IMO; being inconsistent about it like this is wacky).

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 051f06692c..e16da68dec 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -351,6 +351,10 @@  int ff_h264_decode_seq_parameter_set(GetBitContext *gb, AVCodecContext *avctx,
     }
     memcpy(sps->data, gb->buffer, sps->data_size);
 
+    // Re-add the removed stop bit (may be used by hwaccels).
+    if (!(gb->size_in_bits & 7) && sps->data_size < sizeof(sps->data))
+        sps->data[sps->data_size++] = 0x80;
+
     profile_idc           = get_bits(gb, 8);
     constraint_set_flags |= get_bits1(gb) << 0;   // constraint_set0_flag
     constraint_set_flags |= get_bits1(gb) << 1;   // constraint_set1_flag
@@ -775,6 +779,10 @@  int ff_h264_decode_picture_parameter_set(GetBitContext *gb, AVCodecContext *avct
     }
     memcpy(pps->data, gb->buffer, pps->data_size);
 
+    // Re-add the removed stop bit (may be used by hwaccels).
+    if (!(bit_length & 7) && pps->data_size < sizeof(pps->data))
+        pps->data[pps->data_size++] = 0x80;
+
     pps->sps_id = get_ue_golomb_31(gb);
     if ((unsigned)pps->sps_id >= MAX_SPS_COUNT ||
         !ps->sps_list[pps->sps_id]) {