[FFmpeg-devel] [PATCH] avcodec: Add AVClass to AVCodecParameters

[Hendrik Leppkes]

On Tue, May 17, 2016 at 9:04 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Tue, May 17, 2016 at 2:29 PM, Hendrik Leppkes <h.leppkes at gmail.com>
> wrote:
>
>> On Tue, May 17, 2016 at 7:46 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Tue, May 17, 2016 at 1:24 PM, Hendrik Leppkes <h.leppkes at gmail.com>
>> > wrote:
>> >
>> >> On Tue, May 17, 2016 at 7:22 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > On Tue, May 17, 2016 at 12:34 PM, Hendrik Leppkes <
>> h.leppkes at gmail.com>
>> >> > wrote:
>> >> >
>> >> >> On Tue, May 17, 2016 at 6:26 PM, Ronald S. Bultje <
>> rsbultje at gmail.com>
>> >> >> wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > On Tue, May 17, 2016 at 11:18 AM, Michael Niedermayer <
>> >> >> > michael at niedermayer.cc> wrote:
>> >> >> >
>> >> >> >> AVCodecContext contains all the fields that AVCodecParameters has.
>> >> >> >> Examples are width, height, bitrate, profile, sample_rate,
>> channels
>> >> >> >>
>> >> >> >> In AVCodecContext these fields are accessible through AVOptions
>> >> >> >> in AVCodecParameters these fields are not accessible through
>> >> AVOptions
>> >> >> >> and your code will crash mysteriously if you try to access them
>> that
>> >> >> >> way
>> >> >> >>
>> >> >> >> This is a inconsistency that was not there before
>> >> >> >
>> >> >> >
>> >> >> > So, at the risk of going semi-offtopic, let me take a different
>> angle
>> >> at
>> >> >> > this problem.
>> >> >> >
>> >> >> > AVCodecParameter is supposed to be an intermediate layer that
>> removes
>> >> the
>> >> >> > need for lavf to access lavc structs like AVCodecContext, right?
>> >> >> >
>> >> >> > So, I worked (a long long time ago) on this software called
>> GStreamer,
>> >> >> and
>> >> >> > specifically, one of the plugins I worked on early on was
>> gst-ffmpeg.
>> >> I
>> >> >> > loved working on it because it practically added so many codecs to
>> the
>> >> >> > player which were previously unsupported. Yay! Now, the API back
>> then
>> >> was
>> >> >> > horrible. You had an AVCodecContext (no AVOptions) and there were
>> >> >> > approximately 100 million fields that could be set. Most were
>> >> >> undocumented,
>> >> >> > and it was totally unclear which needed to be set. For example, to
>> >> play a
>> >> >> > WMA file, you might have to set block_align, but for other audio
>> >> codecs,
>> >> >> > you do not [1].
>> >> >> >
>> >> >> > For video, sometimes you would have to set extradata (e.g. for
>> SVQ3),
>> >> but
>> >> >> > other times, you would not. Huffyuv required bits_per_sample, but
>> h264
>> >> >> did
>> >> >> > not.
>> >> >> >
>> >> >> > So how do you write generic user-facing code where the demuxer and
>> >> >> decoder
>> >> >> > actually are separate entities and merely interact over a
>> serialized
>> >> >> > communication channel? You can't serialize AVCodecContext. I mean,
>> you
>> >> >> > could, but that's a terrible idea. And nothing documented which
>> fields
>> >> >> were
>> >> >> > required to be set and which weren't.
>> >> >> >
>> >> >> > I was hoping that AVCodecParameters would solve this. Ideally, it
>> >> would
>> >> >> be
>> >> >> > a dictionary of fields that are to be shared between demuxer and
>> >> decoder
>> >> >> > (or codec and muxer). But I think it has sort of strayed off of
>> that
>> >> >> intent
>> >> >> > and is just a flat minimized AVCodecContext as it existed in 2003.
>> >> >> >
>> >> >> > Can we fix this? I think AVOptions would be ideally suited to fix
>> >> this.
>> >> >> > AVClass maybe helps also. But if introspection suggests I can set
>> >> >> > block_align on the h264 decoder, or width on an audio decoder, or
>> who
>> >> >> knows
>> >> >> > what. Then I don't think AVOptions or AVClass in AVCodecParameters
>> >> helps
>> >> >> at
>> >> >> > all. It just creates a minimized AVCodecContext from 2003 that will
>> >> >> slowly
>> >> >> > become the current AVCodecContext, which is not all that useful for
>> >> >> people
>> >> >> > that don't use ffmpeg.exe...
>> >> >> >
>> >> >> >
>> >> >>
>> >> >> This is C, I don't want to have to run introspection on an object to
>> >> >> find out what I can set.
>> >> >> A clean and doumented struct is a far more effective way to get this
>> >> >> information across, which is also why I don't want AVOptions for such
>> >> >> a struct, if its not needed to set private options, it would just
>> >> >> double the need for docs (both in doxy and in the AVOption
>> >> >> definition), and to figure out which fields actually exist, you still
>> >> >> have to read the struct or a comment in the header file - so why not
>> >> >> use the struct in the first place?
>> >> >>
>> >> >> Why use something "generic" with a dict or some weirdness, which
>> loses
>> >> >> the strong typing and self-documentation of such a struct? Sounds
>> like
>> >> >> you want to be working in another language, honestly. :)
>> >> >> AVOption will never "know" which fields a particular codec will need,
>> >> >> so that point is not going to get solved either way.
>> >> >
>> >> >
>> >> > It's not for us (ffmpeg.c developers), it's for people using the
>> ffmpeg
>> >> > libraries on other language platforms (e.g. python).
>> >>
>> >> Maybe the ports to those other languages should devise an interface
>> >> appropriate for that language, and we should keep a clean C interface?
>> >
>> >
>> > I suppose. Do we document which fields need to be set without trial and
>> > error or manually inspecting the decoder source code?
>> >
>>
>> The fields are documented as video and audio appropriately, but
>> otherwise since this information depends on every single decoder,
>> there is no way to really document this properly.
>> Its a much shorter list than AVCodecContext used to have, so its much
>> easier to figure out and fill.
>>
>> Afterall, if you want to go down on a level this low and fill this
>> struct manually, we do expect you do know a tiny bit about codecs, so
>> filling any of these fields should not be a huge problem.
>> If you just use avformat->avcodec, you do not need to concern yourself
>> with the contents of this struct.
>>
>> We should of course always strife to improve the documentation of our
>> public interface to outline which fields are unlikely to be required
>> to be set (only a small subset of codecs, like raw codecs often
>> needing more than compressed ones), or never required to be set, and
>> just used to export a bit of extra detail from avformat.
>
>
> But isn't this knowledge critical if we want to call avformat and avcodec
> "separated"?
>
> We currently just replace AVCodecContext with AVCodecParameters, which is
> clearly an improvement, but is really still suffering from the same
> fundamental problem, just at a smaller scale.
>

The main point is that there will always be codecs that require more
fields than others, and that is a fundamental thing one has to
understand and accept (and know that we cannot possibly document every
codecs need)
Everything else we can just write docs for.

- Hendrik