On 4/10/2019 2:22 AM, Hendrik Leppkes wrote: > On Wed, Apr 10, 2019 at 3:28 AM James Almer <jamr...@gmail.com> wrote: >>> >>>> Two thirds of SPS is not hard to implement, so i really don't understand >>>> why you're so adamantly against it. >>> >>> I’m adamant about re-using code between libavcodec and libavformat. >>> Re-using code is _good_ >> >> So lets do what i suggested in a previous email if re-implementing sps >> in libavformat is not ok in your opinion: Add an >> avpriv_h264_decode_seq_parameter_set() function that internally calls >> ff_h264_decode_seq_parameter_set(), including proper AVBufferRef >> cleaning at the end with a call to ff_h264_ps_uninit(), and that either >> returns the six values using pointers parameters, or takes a pointer to >> a new, small struct as parameter which will be allocated at runtime >> (thus avoiding storing it on stack within libavformat, and tying to the >> ABI) which contains at least those six values in question. >> The reason i suggest a new small struct is to avoid using H264ParamSets >> for this, which would imply direct access to ps->sps fields within >> libavformat, and thus locking everything in its current offset. >> >> See how avpriv_ac3_parse_header() and av_ac3_parse_header() wrap >> ff_ac3_parse_header() and each do either of the two options above. > > As mentioned in another thread on this topic already, I agree with > this. If you insist on re-use so strongly, then lets not cement more > internal structs into the ABI, especially some that we worked on to > get out of the ABI recently in the first place, and define a clean > wrapper API that takes simple byte pointers as input and gives you a > simplified public struct back.
I could agree to use H264ParamSets if as part of this set every single SPS value defined in the latest version of the spec is added to "struct SPS" and parsed by ff_h264_decode_seq_parameter_set() instead of being discarded or stored exclusively in the AVCodecContext as it's the case right now. The existing ones should also be double checked, seeing how i just now found one with the wrong size (and sent a patch to fix it). That way there will be nothing to expand in the future, and locking fields to their current offset will not be an issue. Ideally, the same would be done to "struct PPS", but as long as ps->pps is not accessed from libavformat, it should be ok. This of course will still need H264ParamSets to be allocated by the new function to not let libavformat store it in stack, as its size must not be part of the ABI, only the field's offsets. It would also require ff_h264_ps_uninit to be made avpriv as it will need to be called from outside the sps wrapper. So it's considerably a lot more work than just using a new simple struct. In all cases, be it a new simple struct or H264ParamSets, what must absolutely not be used as a parameter is GetBitContext. Use instead a data and size parameters, and allocate a GetBitContext context within the wrapper. > > - Hendrik _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
