diff mbox

[FFmpeg-devel,v1] avcodec/h264_metadata_bsf: Fix user data failed to insert in case no SPSs NAL for global headers

Message ID 20191226005551.7652-1-lance.lmwang@gmail.com
State New
Headers show

Commit Message

Limin Wang Dec. 26, 2019, 12:55 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

FLV, MP4... will enable global_header default and place SPSs headers in extradata
instead of every keyframe. So it'll failed to insert user data unregisted
for no SPSs NAL after first AU without the patch.

Please test it with below command:
./ffmpeg -f lavfi -i testsrc -c:v libx264 -g 25 \
    -bsf:v h264_metadata=sei_user_data=086f3693-b7b3-4f2c-9653-21492feee5b8+hello \
    -frames:v 150 test.mp4

After applied the patch, you'll get the user data for every keyframe with below command:
./ffmpeg -i test.mp4 -vf showinfo -frames:v 150 -f null -

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/h264_metadata_bsf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Mark Thompson Dec. 27, 2019, 11:30 p.m. UTC | #1
On 26/12/2019 00:55, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> FLV, MP4... will enable global_header default and place SPSs headers in extradata
> instead of every keyframe. So it'll failed to insert user data unregisted
> for no SPSs NAL after first AU without the patch.
> 
> Please test it with below command:
> ./ffmpeg -f lavfi -i testsrc -c:v libx264 -g 25 \
>     -bsf:v h264_metadata=sei_user_data=086f3693-b7b3-4f2c-9653-21492feee5b8+hello \
>     -frames:v 150 test.mp4
> 
> After applied the patch, you'll get the user data for every keyframe with below command:
> ./ffmpeg -i test.mp4 -vf showinfo -frames:v 150 -f null -
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavcodec/h264_metadata_bsf.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index 5de74be9d6..9690ca433b 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -279,7 +279,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
>  {
>      H264MetadataContext *ctx = bsf->priv_data;
>      CodedBitstreamFragment *au = &ctx->access_unit;
> -    int err, i, j, has_sps;
> +    int err, i, j, has_sps, is_keyframe = 0;
>      H264RawAUD aud;
>  
>      err = ff_bsf_get_packet_ref(bsf, pkt);
> @@ -359,11 +359,13 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
>                  goto fail;
>              has_sps = 1;
>          }
> +        if (au->units[i].type == H264_NAL_IDR_SLICE)
> +            is_keyframe = 1;
>      }
>  
>      // Only insert the SEI in access units containing SPSs, and also
>      // unconditionally in the first access unit we ever see.
> -    if (ctx->sei_user_data && (has_sps || !ctx->done_first_au)) {
> +    if (ctx->sei_user_data && (has_sps || !ctx->done_first_au || is_keyframe)) {
>          H264RawSEIPayload payload = {
>              .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
>          };
> 

This was avoided originally because identifying key frames (or really, seek points) in an H.264 stream is really hard.  I don't particularly like the idea of having it work in some streams but not others, depending on exactly how the stream was encoded - hence the original decision to only include it in access units which were already distinguished as flat-file seek-points by having the parameter sets in them.

I guess I'm not against this in itself, but please add more of a comment in the code explaining the rationale and noting the cases which are not included.  (And if you can cover more cases, such as recovery points, then that would be even better.)

Thanks,

- Mark
Limin Wang Dec. 28, 2019, 2:05 a.m. UTC | #2
On Fri, Dec 27, 2019 at 11:30:34PM +0000, Mark Thompson wrote:
> On 26/12/2019 00:55, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > FLV, MP4... will enable global_header default and place SPSs headers in extradata
> > instead of every keyframe. So it'll failed to insert user data unregisted
> > for no SPSs NAL after first AU without the patch.
> > 
> > Please test it with below command:
> > ./ffmpeg -f lavfi -i testsrc -c:v libx264 -g 25 \
> >     -bsf:v h264_metadata=sei_user_data=086f3693-b7b3-4f2c-9653-21492feee5b8+hello \
> >     -frames:v 150 test.mp4
> > 
> > After applied the patch, you'll get the user data for every keyframe with below command:
> > ./ffmpeg -i test.mp4 -vf showinfo -frames:v 150 -f null -
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavcodec/h264_metadata_bsf.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> > index 5de74be9d6..9690ca433b 100644
> > --- a/libavcodec/h264_metadata_bsf.c
> > +++ b/libavcodec/h264_metadata_bsf.c
> > @@ -279,7 +279,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
> >  {
> >      H264MetadataContext *ctx = bsf->priv_data;
> >      CodedBitstreamFragment *au = &ctx->access_unit;
> > -    int err, i, j, has_sps;
> > +    int err, i, j, has_sps, is_keyframe = 0;
> >      H264RawAUD aud;
> >  
> >      err = ff_bsf_get_packet_ref(bsf, pkt);
> > @@ -359,11 +359,13 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
> >                  goto fail;
> >              has_sps = 1;
> >          }
> > +        if (au->units[i].type == H264_NAL_IDR_SLICE)
> > +            is_keyframe = 1;
> >      }
> >  
> >      // Only insert the SEI in access units containing SPSs, and also
> >      // unconditionally in the first access unit we ever see.
> > -    if (ctx->sei_user_data && (has_sps || !ctx->done_first_au)) {
> > +    if (ctx->sei_user_data && (has_sps || !ctx->done_first_au || is_keyframe)) {
> >          H264RawSEIPayload payload = {
> >              .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
> >          };
> > 
> 
> This was avoided originally because identifying key frames (or really, seek points) in an H.264 stream is really hard.  I don't particularly like the idea of having it work in some streams but not others, depending on exactly how the stream was encoded - hence the original decision to only include it in access units which were already distinguished as flat-file seek-points by having the parameter sets in them.
> 
> I guess I'm not against this in itself, but please add more of a comment in the code explaining the rationale and noting the cases which are not included.  (And if you can cover more cases, such as recovery points, then that would be even better.)

FFmpeg CLI can't support change the parameter on the fly, by my case, the user
data string will be updated periodically. For the stream, like rtmp, no SPSs 
header in the stream, so the user data failed to insert anymore. For IDR_SLICE
is good to cover most condition, so I did not add recovery points as the user
data don't depend on it in fact.  If you think it's needed, I'll add more checking.

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> 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".
diff mbox

Patch

diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
index 5de74be9d6..9690ca433b 100644
--- a/libavcodec/h264_metadata_bsf.c
+++ b/libavcodec/h264_metadata_bsf.c
@@ -279,7 +279,7 @@  static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
 {
     H264MetadataContext *ctx = bsf->priv_data;
     CodedBitstreamFragment *au = &ctx->access_unit;
-    int err, i, j, has_sps;
+    int err, i, j, has_sps, is_keyframe = 0;
     H264RawAUD aud;
 
     err = ff_bsf_get_packet_ref(bsf, pkt);
@@ -359,11 +359,13 @@  static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
                 goto fail;
             has_sps = 1;
         }
+        if (au->units[i].type == H264_NAL_IDR_SLICE)
+            is_keyframe = 1;
     }
 
     // Only insert the SEI in access units containing SPSs, and also
     // unconditionally in the first access unit we ever see.
-    if (ctx->sei_user_data && (has_sps || !ctx->done_first_au)) {
+    if (ctx->sei_user_data && (has_sps || !ctx->done_first_au || is_keyframe)) {
         H264RawSEIPayload payload = {
             .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
         };