| Message ID | 20210320080955.97660-1-sharpbai@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/h264_metadata: add change pic_order_cnt_type option | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
sharpbai: > Some encoder set pic_order_cnt_type=0 when not using bframe, > such as h264_videotoolbox. It may cause that some hardware decoder > delays output frames (buffer up to 18frames), such as MediaCodec > decoder before Android 11. > > Setting pic_order_cnt_type=2 indicates that the picture order could not > be reversed, it will minimize the decoding delay on some decoder > implementations. > > Signed-off-by: sharpbai <sharpbai@gmail.com> > --- > libavcodec/cbs_h264.h | 1 + > libavcodec/cbs_h264_syntax_template.c | 16 ++++++++++++++++ > libavcodec/h264_metadata_bsf.c | 9 +++++++++ > 3 files changed, 26 insertions(+) > > diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h > index 9f7c2a0d30..00fe5178ea 100644 > --- a/libavcodec/cbs_h264.h > +++ b/libavcodec/cbs_h264.h > @@ -136,6 +136,7 @@ typedef struct H264RawSPS { > > uint8_t log2_max_frame_num_minus4; > uint8_t pic_order_cnt_type; > + uint8_t pic_order_cnt_type_write; > uint8_t log2_max_pic_order_cnt_lsb_minus4; > uint8_t delta_pic_order_always_zero_flag; > int32_t offset_for_non_ref_pic; > diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c > index b65460996b..4a8a8442dd 100644 > --- a/libavcodec/cbs_h264_syntax_template.c > +++ b/libavcodec/cbs_h264_syntax_template.c > @@ -324,9 +324,17 @@ static int FUNC(sps)(CodedBitstreamContext *ctx, RWContext *rw, > } > > ue(log2_max_frame_num_minus4, 0, 12); > + #ifdef READ > ue(pic_order_cnt_type, 0, 2); > + #else > + ue(pic_order_cnt_type_write, 0, 2); > + #endif This is wrong. Instead you should make the SPS that you are modifying writable and update it. (The h264_metadata bsf uses separate CodedBitstreamContexts for input and output and the SPS is normally shared between them, so if someone wants to modify a field one needs to make it writable which makes a copy of the NAL unit (the reason it is currently not done is because the fields that are changed are not used for parsing at all, but pic_order_cnt_type is). See h264_redundant_pps for an example of this.) (Btw: What you are doing here will break writing H.264 SPS if pic_order_cnt_type_write isn't set (unless pic_order_cnt_type is zero). This can e.g. happen for the aforementioned h264_redundant_pps BSF.) > > + #ifdef READ > if (current->pic_order_cnt_type == 0) { > + #else > + if (current->pic_order_cnt_type_write == 0) { > + #endif > ue(log2_max_pic_order_cnt_lsb_minus4, 0, 12); > } else if (current->pic_order_cnt_type == 1) { > flag(delta_pic_order_always_zero_flag); > @@ -1265,13 +1273,21 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw, > if (idr_pic_flag) > ue(idr_pic_id, 0, 65535); > > + #ifdef READ > if (sps->pic_order_cnt_type == 0) { > + #else > + if (sps->pic_order_cnt_type_write == 0) { > + #endif > ub(sps->log2_max_pic_order_cnt_lsb_minus4 + 4, pic_order_cnt_lsb); > if (pps->bottom_field_pic_order_in_frame_present_flag && > !current->field_pic_flag) > se(delta_pic_order_cnt_bottom, INT32_MIN + 1, INT32_MAX); > > + #ifdef READ > } else if (sps->pic_order_cnt_type == 1) { > + #else > + } else if (sps->pic_order_cnt_type_write == 1) { > + #endif > if (!sps->delta_pic_order_always_zero_flag) { > se(delta_pic_order_cnt[0], INT32_MIN + 1, INT32_MAX); > if (pps->bottom_field_pic_order_in_frame_present_flag && > diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c > index cef054bd65..c21e477841 100644 > --- a/libavcodec/h264_metadata_bsf.c > +++ b/libavcodec/h264_metadata_bsf.c > @@ -59,6 +59,7 @@ typedef struct H264MetadataContext { > AVRational sample_aspect_ratio; > > int overscan_appropriate_flag; > + int pic_order_cnt_type; > > int video_format; > int video_full_range_flag; > @@ -95,6 +96,11 @@ static int h264_metadata_update_sps(AVBSFContext *bsf, > int need_vui = 0; > int crop_unit_x, crop_unit_y; > > + if (ctx->pic_order_cnt_type != -1) { > + sps->pic_order_cnt_type_write = ctx->pic_order_cnt_type; > + } else { > + sps->pic_order_cnt_type_write = sps->pic_order_cnt_type; > + } This may break valid files and is therefore completely unsafe and should IMO not be in this bsf. Have you tested whether setting the number of reorder frames (which is the typical way to set the delay in H.264) would be enough for your usecase? I have already made a patch for this here: https://github.com/mkver/FFmpeg/commit/6f6de261b30ef493f65af293c66798fe625ea743, but it is not based upon current git master. Just remove the check (which relies on a function not added in git master (introduced here: https://github.com/mkver/FFmpeg/commit/29ff08b47ca63faff250802cdf743e2ac89d34f5)). My usecase for this patch is slightly different from yours though: When the input doesn't have reorder frames set, one has to guess it if the container does not provide dts (think of Matroska). And said guess can be wrong. > if (ctx->sample_aspect_ratio.num && ctx->sample_aspect_ratio.den) { > // Table E-1. > static const AVRational sar_idc[] = { > @@ -689,6 +695,9 @@ static const AVOption h264_metadata_options[] = { > OFFSET(overscan_appropriate_flag), AV_OPT_TYPE_INT, > { .i64 = -1 }, -1, 1, FLAGS }, > > + { "pic_order_cnt_type", "Set pic_order_cnt_type", > + OFFSET(pic_order_cnt_type), AV_OPT_TYPE_INT, > + { .i64 = -1 }, -1, 2, FLAGS }, > { "video_format", "Set video format (table E-2)", > OFFSET(video_format), AV_OPT_TYPE_INT, > { .i64 = -1 }, -1, 7, FLAGS}, >
To Andreas Rheinhardt, > This is wrong. Instead you should make the SPS that you are modifying > writable and update it. (The h264_metadata bsf uses separate > CodedBitstreamContexts for input and output and the SPS is normally > shared between them, so if someone wants to modify a field one needs to > make it writable which makes a copy of the NAL unit (the reason it is > currently not done is because the fields that are changed are not used > for parsing at all, but pic_order_cnt_type is). See h264_redundant_pps > for an example of this.) > > (Btw: What you are doing here will break writing H.264 SPS if > pic_order_cnt_type_write isn't set (unless pic_order_cnt_type is zero). > This can e.g. happen for the aforementioned h264_redundant_pps BSF.) Got it. I will implement a new bsf using the The right way. > This may break valid files and is therefore completely unsafe and should > IMO not be in this bsf. > Have you tested whether setting the number of reorder frames (which is > the typical way to set the delay in H.264) would be enough for your > usecase? I have already made a patch for this here: > https://github.com/mkver/FFmpeg/commit/6f6de261b30ef493f65af293c66798fe625ea743, > but it is not based upon current git master. Just remove the check > (which relies on a function not added in git master (introduced here: > https://github.com/mkver/FFmpeg/commit/29ff08b47ca63faff250802cdf743e2ac89d34f5)). > My usecase for this patch is slightly different from yours though: When > the input doesn't have reorder frames set, one has to guess it if the > container does not provide dts (think of Matroska). And said guess can > be wrong. I have read the patch above. It could not solve my case. In my case, the keypoint is we do know the encoder will not reorder the encoded frame, but the decoder could not recognize it. Then for safety some decoders will allocate a decode buffer larger than we need, which result in decoding delay. At realtime scenario, this delay is not accepted. The patch above will not accept a smaller value (such as 0) to minimize the delay, so it could not solve my case. Yes, there is a risk of breaking valid files. I will raise an error when detecting a reordered frame on setting pic_order_cnt_type to 2 to avoid breaking valid files. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
sharpbai@gmail.com: > To Andreas Rheinhardt, > >> This is wrong. Instead you should make the SPS that you are modifying >> writable and update it. (The h264_metadata bsf uses separate >> CodedBitstreamContexts for input and output and the SPS is normally >> shared between them, so if someone wants to modify a field one needs to >> make it writable which makes a copy of the NAL unit (the reason it is >> currently not done is because the fields that are changed are not used >> for parsing at all, but pic_order_cnt_type is). See h264_redundant_pps >> for an example of this.) >> >> (Btw: What you are doing here will break writing H.264 SPS if >> pic_order_cnt_type_write isn't set (unless pic_order_cnt_type is zero). >> This can e.g. happen for the aforementioned h264_redundant_pps BSF.) > > Got it. I will implement a new bsf using the The right way. > > >> This may break valid files and is therefore completely unsafe and should >> IMO not be in this bsf. >> Have you tested whether setting the number of reorder frames (which is >> the typical way to set the delay in H.264) would be enough for your >> usecase? I have already made a patch for this here: >> https://github.com/mkver/FFmpeg/commit/6f6de261b30ef493f65af293c66798fe625ea743, >> but it is not based upon current git master. Just remove the check >> (which relies on a function not added in git master (introduced here: >> https://github.com/mkver/FFmpeg/commit/29ff08b47ca63faff250802cdf743e2ac89d34f5)). >> My usecase for this patch is slightly different from yours though: When >> the input doesn't have reorder frames set, one has to guess it if the >> container does not provide dts (think of Matroska). And said guess can >> be wrong. > > I have read the patch above. It could not solve my case. In my case, the > keypoint is we do know the encoder will not reorder the encoded frame, > but the decoder could not recognize it. Then for safety some decoders will > allocate a decode buffer larger than we need, which result in decoding delay. > At realtime scenario, this delay is not accepted. The patch above will not > accept a smaller value (such as 0) to minimize the delay, so it could not > solve my case. What does "will not accept a smaller value (such as 0)" mean? Does it error out? What would happen if you set sps->vui.max_dec_frame_buffering = FFMAX(ctx->reorder_frames, sps->max_num_ref_frames); instead of what it is set to know (if the bitstream restriction flags aren't there, max_dec_frame_buffering is inferred to be 16 (the maximum size) and will stay 16 with the old code). Notice that the change I am suggesting here is not safe in general. > > Yes, there is a risk of breaking valid files. I will raise an error when detecting > a reordered frame on setting pic_order_cnt_type to 2 to avoid breaking valid > files. > >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
To Andreas Rheinhardt, > What does "will not accept a smaller value (such as 0)" mean? As your said, the max_dec_frame_buffering is limited to 16. Set it to 0 (on condition that we do know there is no reordered frames) is not possible. > Does it error out? It is safe and will not break any files. It's not wrong and solves your mentioned case. :) > Notice that the change I am suggesting here is not safe in general. Yes. It is not safe unless you know what you are doing. For safety this feature must have some mechanism to detect the unsafe usage. Let me introduce what my case is. First we encode using h264_videotoolbox setting -bf to 0 (or not set. -bf default is 0) for speedy and lower power consumption. Then we send the encoded bytestream to the other client as quickly as possible. Then we decode using Android MediaCodec for speedy and lower consumption. But we have found that the decoder output delays about 18 frames, which is above 1 seconds at 15 fps. This is not acceptable. Now we have changed the encoded bytestream before sending, which makes the decoding delay down to 2 frames. We could hardly change videotoolbox and MediaCodec. So the unsafe way in general turns to be effective (or only, up to now) way at realtime scenario.
sharpbai@gmail.com: > To Andreas Rheinhardt, > >> What does "will not accept a smaller value (such as 0)" mean? > > As your said, the max_dec_frame_buffering is limited to 16. Set > it to 0 (on condition that we do know there is no reordered frames) > is not possible. > Why is this not possible? I just told you how you could set max_dec_frame_buffering to a lower value. It seems you haven't tried it. >> Does it error out? > > It is safe and will not break any files. It's not wrong and solves > your mentioned case. :) > >> Notice that the change I am suggesting here is not safe in general. > > Yes. It is not safe unless you know what you are doing. For safety > this feature must have some mechanism to detect the unsafe usage. > > Let me introduce what my case is. First we encode using h264_videotoolbox > setting -bf to 0 (or not set. -bf default is 0) for speedy and lower > power consumption. Then we send the encoded bytestream to the other client > as quickly as possible. Then we decode using Android MediaCodec for speedy > and lower consumption. But we have found that the decoder output delays > about 18 frames, which is above 1 seconds at 15 fps. This is not acceptable. > Now we have changed the encoded bytestream before sending, which makes the > decoding delay down to 2 frames. > > We could hardly change videotoolbox and MediaCodec. So the unsafe way in > general turns to be effective (or only, up to now) way at realtime scenario. > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
To Andreas Rheinhardt, > Why is this not possible? I just told you how you could set > max_dec_frame_buffering to a lower value. It seems you haven't tried it. Ahh...I thought what you care about is the unsafety of directly modifing num_reorder_frames and num_reorder_frames to a lower value. We have tested set max_dec_frame_buffering to 0 and num_reorder_frames to 0. The result is the same as set pic_order_cnt_type to 2. Setting max_dec_frame_buffering requires to modify sps only while setting pic_order_cnt_type requires to modify every slice. I think your method is better. Btw, for safety you could compare pic_order_cnt_lsb and frame_num in every slice header. The difference should not be larger than (num_reorder_frames * 2). Or an error should be generated.
sharpbai@gmail.com: > To Andreas Rheinhardt, > >> Why is this not possible? I just told you how you could set >> max_dec_frame_buffering to a lower value. It seems you haven't tried it. > > Ahh...I thought what you care about is the unsafety of directly modifing > num_reorder_frames and num_reorder_frames to a lower value. > It is unsafe in the sense that it can lead to invalid and broken files. But it is way safer than actually modifying the slice headers, because the latter is absolutely irreversible, whereas one just needs to set big enough values to fix files broken by setting too low values for reorder_frames and max_dec_frame_buffering. For this a warning in the documentation that wrong values might break files should suffice. > We have tested set max_dec_frame_buffering to 0 and num_reorder_frames to 0. > The result is the same as set pic_order_cnt_type to 2. > Just for confirmation: Setting num_reorder_frames to zero is not enough? > Setting max_dec_frame_buffering requires to modify sps only while setting > pic_order_cnt_type requires to modify every slice. I think your method > is better. > > Btw, for safety you could compare pic_order_cnt_lsb and frame_num in every > slice header. The difference should not be larger than (num_reorder_frames * 2). > Or an error should be generated. > This is wrong: pic_order_cnt_lsb is typically incremented by two per frame (but it can be different), whereas frame_num is only incremented after a reference picture and only by 1. Furthermore, both these values are only coded modulo a certain power of two and these powers can be different. - Andreas
To Andreas Rheinhardt, > It is unsafe in the sense that it can lead to invalid and broken files. > But it is way safer than actually modifying the slice headers, because > the latter is absolutely irreversible, whereas one just needs to set big > enough values to fix files broken by setting too low values for > reorder_frames and max_dec_frame_buffering. For this a warning in the > documentation that wrong values might break files should suffice. Got it. > Just for confirmation: Setting num_reorder_frames to zero is not enough? On my tested devices setting num_reorder_frames to zero is enough. But I can't ensure it will be effective on every decoder. > This is wrong: pic_order_cnt_lsb is typically incremented by two per > frame (but it can be different), whereas frame_num is only incremented > after a reference picture and only by 1. Furthermore, both these values > are only coded modulo a certain power of two and these powers can be > different. I have found that only on macOS 11 pic_order_cnt_lsb is incremented by two while on iOS 14 or other Android devices I have tested pic_order_cnt_lsb is incremented by one. You could try to generate a bytestream on iOS to verify it. sharpbai
diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h index 9f7c2a0d30..00fe5178ea 100644 --- a/libavcodec/cbs_h264.h +++ b/libavcodec/cbs_h264.h @@ -136,6 +136,7 @@ typedef struct H264RawSPS { uint8_t log2_max_frame_num_minus4; uint8_t pic_order_cnt_type; + uint8_t pic_order_cnt_type_write; uint8_t log2_max_pic_order_cnt_lsb_minus4; uint8_t delta_pic_order_always_zero_flag; int32_t offset_for_non_ref_pic; diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c index b65460996b..4a8a8442dd 100644 --- a/libavcodec/cbs_h264_syntax_template.c +++ b/libavcodec/cbs_h264_syntax_template.c @@ -324,9 +324,17 @@ static int FUNC(sps)(CodedBitstreamContext *ctx, RWContext *rw, } ue(log2_max_frame_num_minus4, 0, 12); + #ifdef READ ue(pic_order_cnt_type, 0, 2); + #else + ue(pic_order_cnt_type_write, 0, 2); + #endif + #ifdef READ if (current->pic_order_cnt_type == 0) { + #else + if (current->pic_order_cnt_type_write == 0) { + #endif ue(log2_max_pic_order_cnt_lsb_minus4, 0, 12); } else if (current->pic_order_cnt_type == 1) { flag(delta_pic_order_always_zero_flag); @@ -1265,13 +1273,21 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw, if (idr_pic_flag) ue(idr_pic_id, 0, 65535); + #ifdef READ if (sps->pic_order_cnt_type == 0) { + #else + if (sps->pic_order_cnt_type_write == 0) { + #endif ub(sps->log2_max_pic_order_cnt_lsb_minus4 + 4, pic_order_cnt_lsb); if (pps->bottom_field_pic_order_in_frame_present_flag && !current->field_pic_flag) se(delta_pic_order_cnt_bottom, INT32_MIN + 1, INT32_MAX); + #ifdef READ } else if (sps->pic_order_cnt_type == 1) { + #else + } else if (sps->pic_order_cnt_type_write == 1) { + #endif if (!sps->delta_pic_order_always_zero_flag) { se(delta_pic_order_cnt[0], INT32_MIN + 1, INT32_MAX); if (pps->bottom_field_pic_order_in_frame_present_flag && diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c index cef054bd65..c21e477841 100644 --- a/libavcodec/h264_metadata_bsf.c +++ b/libavcodec/h264_metadata_bsf.c @@ -59,6 +59,7 @@ typedef struct H264MetadataContext { AVRational sample_aspect_ratio; int overscan_appropriate_flag; + int pic_order_cnt_type; int video_format; int video_full_range_flag; @@ -95,6 +96,11 @@ static int h264_metadata_update_sps(AVBSFContext *bsf, int need_vui = 0; int crop_unit_x, crop_unit_y; + if (ctx->pic_order_cnt_type != -1) { + sps->pic_order_cnt_type_write = ctx->pic_order_cnt_type; + } else { + sps->pic_order_cnt_type_write = sps->pic_order_cnt_type; + } if (ctx->sample_aspect_ratio.num && ctx->sample_aspect_ratio.den) { // Table E-1. static const AVRational sar_idc[] = { @@ -689,6 +695,9 @@ static const AVOption h264_metadata_options[] = { OFFSET(overscan_appropriate_flag), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 1, FLAGS }, + { "pic_order_cnt_type", "Set pic_order_cnt_type", + OFFSET(pic_order_cnt_type), AV_OPT_TYPE_INT, + { .i64 = -1 }, -1, 2, FLAGS }, { "video_format", "Set video format (table E-2)", OFFSET(video_format), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 7, FLAGS},
Some encoder set pic_order_cnt_type=0 when not using bframe, such as h264_videotoolbox. It may cause that some hardware decoder delays output frames (buffer up to 18frames), such as MediaCodec decoder before Android 11. Setting pic_order_cnt_type=2 indicates that the picture order could not be reversed, it will minimize the decoding delay on some decoder implementations. Signed-off-by: sharpbai <sharpbai@gmail.com> --- libavcodec/cbs_h264.h | 1 + libavcodec/cbs_h264_syntax_template.c | 16 ++++++++++++++++ libavcodec/h264_metadata_bsf.c | 9 +++++++++ 3 files changed, 26 insertions(+)