[FFmpeg-devel] [PATCH] h264: expose stereo_mode from h264 frame packing info

[Joakim Plate]

Michael Niedermayer <michaelni <at> gmx.at> writes:

> 
> >  <at>  <at>  -5033,6 +5036,9  <at>  <at>  static const AVProfile 
profiles[] = {
> >  static const AVOption h264_options[] = {
> >      {"is_avc", "is avc", offsetof(H264Context, is_avc), 
FF_OPT_TYPE_INT, {.i64 = 0}, 0, 1, 0},
> >      {"nal_length_size", "nal_length_size", offsetof(H264Context, 
nal_length_size),
> FF_OPT_TYPE_INT, {.i64 = 0}, 0, 4, 0},
> > +    {"frame_packing_arrangement_cancel_flag", 
"frame_packing_arrangement_cancel_flag",
> offsetof(H264Context, sei_fpa.frame_packing_arrangement_cancel_flag), 
FF_OPT_TYPE_INT, {.i64
> = -1}, -1, 1, 0},
> > +    {"frame_packing_arrangement_type", 
"frame_packing_arrangement_type", offsetof(H264Context,
> sei_fpa.frame_packing_arrangement_type), FF_OPT_TYPE_INT, {.i64 = 0}, 0, 
6, 0},
> > +    {"content_interpretation_type", "content_interpretation_type", 
offsetof(H264Context,
> sei_fpa.content_interpretation_type), FF_OPT_TYPE_INT, {.i64 = 0}, 0, 2, 
0},
> >      {NULL}
> >  };
> 
> how do these interact with frame threads ?
> (as they are not strictly global options and could change per frame)

I'm not fully sure. To be honest. I couldn't really follow the thread logic 
when it came to the H264Context, it seem to be duplicated and copied around. 
Maybe the sei_fpa struct need to be copied around too.

> also exporting the cancel flag seems quite low level, isnt exporting
> the current state enough ?

To be honest, I wasn't really sure if there was a point in exporting these 
options externally at all. The cancel flag is needed however, since 3d mode 
can end at any point in the stream and start again. 

The frame_packing_arrangement_type in the spec does include a 2D mode, but 
that explicitly states it's only valid to use under certain situations. Thus
it felt wrong to force this value to the 2d mode if we detect a 
cancellation.

If the export of this data in matroska string format in metadata is okey, I 
would say we should just drop these internal flags.

> 
> > +    skip_bits(&h->gb, 8*size - (bits - get_bits_left(&h->gb)));
> 
> skip_bits_long()
> 

Ok.

Regarding that. To me it sort of feels that logic should be outside this 
function to benefit all the sei parsers. Now if one of the sei parsers fail, 
we fail the whole sei parsing completely.

/Joakim