[FFmpeg-devel] avformat: add H264 and HEVC support in IVF muxer

Submitted by alx.sukhanov@gmail.com on Oct. 1, 2018, 6:01 p.m.

Details

Message ID 20181001180146.49151-1-alx.sukhanov@gmail.com
State New
Headers show

Commit Message

alx.sukhanov@gmail.com Oct. 1, 2018, 6:01 p.m.
From: Alex Sukhanov <asukhanov@google.com>

---
 libavformat/ivfenc.c | 50 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 12 deletions(-)

Comments

alx.sukhanov@gmail.com Oct. 5, 2018, 8:45 p.m.
On Mon, Oct 1, 2018 at 11:01 AM <alx.sukhanov@gmail.com> wrote:

> From: Alex Sukhanov <asukhanov@google.com>
>
> ---
>  libavformat/ivfenc.c | 50 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
> index 66441a2a43..6410828533 100644
> --- a/libavformat/ivfenc.c
> +++ b/libavformat/ivfenc.c
> @@ -36,19 +36,29 @@ static int ivf_write_header(AVFormatContext *s)
>          return AVERROR(EINVAL);
>      }
>      par = s->streams[0]->codecpar;
> -    if (par->codec_type != AVMEDIA_TYPE_VIDEO ||
> -        !(par->codec_id == AV_CODEC_ID_AV1 ||
> -          par->codec_id == AV_CODEC_ID_VP8 ||
> -          par->codec_id == AV_CODEC_ID_VP9)) {
> -        av_log(s, AV_LOG_ERROR, "Currently only VP8, VP9 and AV1 are
> supported!\n");
> -        return AVERROR(EINVAL);
> -    }
>      avio_write(pb, "DKIF", 4);
>      avio_wl16(pb, 0); // version
>      avio_wl16(pb, 32); // header length
> -    avio_wl32(pb,
> -              par->codec_id == AV_CODEC_ID_VP9 ? AV_RL32("VP90") :
> -              par->codec_id == AV_CODEC_ID_VP8 ? AV_RL32("VP80") :
> AV_RL32("AV01"));
> +    switch (par->codec_id) {
> +      case AV_CODEC_ID_AV1:
> +        avio_wl32(pb, AV_RL32("AV01"));
> +        break;
> +      case AV_CODEC_ID_H264:
> +        avio_wl32(pb, AV_RL32("H264"));
> +        break;
> +      case AV_CODEC_ID_HEVC:
> +        avio_wl32(pb, AV_RL32("HEVC"));
> +        break;
> +      case AV_CODEC_ID_VP8:
> +        avio_wl32(pb, AV_RL32("VP80"));
> +        break;
> +      case AV_CODEC_ID_VP9:
> +        avio_wl32(pb, AV_RL32("VP90"));
> +        break;
> +      default:
> +        av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and
> VP9 and AV1 are supported!\n");
> +        return AVERROR(EINVAL);
> +    }
>      avio_wl16(pb, par->width);
>      avio_wl16(pb, par->height);
>      avio_wl32(pb, s->streams[0]->time_base.den);
> @@ -95,16 +105,32 @@ static int ivf_check_bitstream(struct AVFormatContext
> *s, const AVPacket *pkt)
>      int ret = 1;
>      AVStream *st = s->streams[pkt->stream_index];
>
> -    if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
> +    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
> +                             (AV_RB24(pkt->data) != 0x000001 ||
> +                              (st->codecpar->extradata_size > 0 &&
> +                               st->codecpar->extradata[0] == 1)))
> +            ret = ff_stream_add_bitstream_filter(st, "h264_mp4toannexb",
> NULL);
> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_HEVC) {
> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
> +                             (AV_RB24(pkt->data) != 0x000001 ||
> +                              (st->codecpar->extradata_size > 0 &&
> +                               st->codecpar->extradata[0] == 1)))
> +            ret = ff_stream_add_bitstream_filter(st, "hevc_mp4toannexb",
> NULL);
> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_VP9) {
>          ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
> +    }
>
>      return ret;
>  }
>
>  static const AVCodecTag codec_ivf_tags[] = {
> +    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
> +    { AV_CODEC_ID_H264, MKTAG('H', '2', '6', '4') },
> +    { AV_CODEC_ID_HEVC, MKTAG('H', 'E', 'V', 'C') },
>      { AV_CODEC_ID_VP8,  MKTAG('V', 'P', '8', '0') },
>      { AV_CODEC_ID_VP9,  MKTAG('V', 'P', '9', '0') },
> -    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
> +
>      { AV_CODEC_ID_NONE, 0 }
>  };
>
> --
> 2.19.0.605.g01d371f741-goog
>
>
Can you please take a look on this patch?
Thank you
Mark Thompson Oct. 6, 2018, 5:36 p.m.
On 05/10/18 21:45, Alex Sukhanov wrote:
> On Mon, Oct 1, 2018 at 11:01 AM <alx.sukhanov@gmail.com> wrote:
> 
>> From: Alex Sukhanov <asukhanov@google.com>
>>
>> ---
>>  libavformat/ivfenc.c | 50 +++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 38 insertions(+), 12 deletions(-)
>>
>> diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
>> index 66441a2a43..6410828533 100644
>> --- a/libavformat/ivfenc.c
>> +++ b/libavformat/ivfenc.c
>> @@ -36,19 +36,29 @@ static int ivf_write_header(AVFormatContext *s)
>>          return AVERROR(EINVAL);
>>      }
>>      par = s->streams[0]->codecpar;
>> -    if (par->codec_type != AVMEDIA_TYPE_VIDEO ||
>> -        !(par->codec_id == AV_CODEC_ID_AV1 ||
>> -          par->codec_id == AV_CODEC_ID_VP8 ||
>> -          par->codec_id == AV_CODEC_ID_VP9)) {
>> -        av_log(s, AV_LOG_ERROR, "Currently only VP8, VP9 and AV1 are
>> supported!\n");
>> -        return AVERROR(EINVAL);
>> -    }
>>      avio_write(pb, "DKIF", 4);
>>      avio_wl16(pb, 0); // version
>>      avio_wl16(pb, 32); // header length
>> -    avio_wl32(pb,
>> -              par->codec_id == AV_CODEC_ID_VP9 ? AV_RL32("VP90") :
>> -              par->codec_id == AV_CODEC_ID_VP8 ? AV_RL32("VP80") :
>> AV_RL32("AV01"));
>> +    switch (par->codec_id) {
>> +      case AV_CODEC_ID_AV1:
>> +        avio_wl32(pb, AV_RL32("AV01"));
>> +        break;
>> +      case AV_CODEC_ID_H264:
>> +        avio_wl32(pb, AV_RL32("H264"));
>> +        break;
>> +      case AV_CODEC_ID_HEVC:
>> +        avio_wl32(pb, AV_RL32("HEVC"));
>> +        break;
>> +      case AV_CODEC_ID_VP8:
>> +        avio_wl32(pb, AV_RL32("VP80"));
>> +        break;
>> +      case AV_CODEC_ID_VP9:
>> +        avio_wl32(pb, AV_RL32("VP90"));
>> +        break;
>> +      default:
>> +        av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and
>> VP9 and AV1 are supported!\n");
>> +        return AVERROR(EINVAL);
>> +    }
>>      avio_wl16(pb, par->width);
>>      avio_wl16(pb, par->height);
>>      avio_wl32(pb, s->streams[0]->time_base.den);
>> @@ -95,16 +105,32 @@ static int ivf_check_bitstream(struct AVFormatContext
>> *s, const AVPacket *pkt)
>>      int ret = 1;
>>      AVStream *st = s->streams[pkt->stream_index];
>>
>> -    if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
>> +    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
>> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
>> +                             (AV_RB24(pkt->data) != 0x000001 ||
>> +                              (st->codecpar->extradata_size > 0 &&
>> +                               st->codecpar->extradata[0] == 1)))
>> +            ret = ff_stream_add_bitstream_filter(st, "h264_mp4toannexb",
>> NULL);
>> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_HEVC) {
>> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
>> +                             (AV_RB24(pkt->data) != 0x000001 ||
>> +                              (st->codecpar->extradata_size > 0 &&
>> +                               st->codecpar->extradata[0] == 1)))
>> +            ret = ff_stream_add_bitstream_filter(st, "hevc_mp4toannexb",
>> NULL);
>> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_VP9) {
>>          ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
>> +    }
>>
>>      return ret;
>>  }
>>
>>  static const AVCodecTag codec_ivf_tags[] = {
>> +    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
>> +    { AV_CODEC_ID_H264, MKTAG('H', '2', '6', '4') },
>> +    { AV_CODEC_ID_HEVC, MKTAG('H', 'E', 'V', 'C') },
>>      { AV_CODEC_ID_VP8,  MKTAG('V', 'P', '8', '0') },
>>      { AV_CODEC_ID_VP9,  MKTAG('V', 'P', '9', '0') },
>> -    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
>> +
>>      { AV_CODEC_ID_NONE, 0 }
>>  };
>>
>> --
>> 2.19.0.605.g01d371f741-goog
>>
>>
> Can you please take a look on this patch?

Can you answer the question posed in <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-September/234655.html> about the reasoning for this patch?

In particular, it might be helpful if you could point out any tools/standards which support (or are in future intending to support) this format - in my experience pretty much everything dealing with H.26[45] raw streams uses the Annex B format.

Thanks,

- Mark
alx.sukhanov@gmail.com Oct. 11, 2018, 7:58 p.m.
On Sat, Oct 6, 2018 at 10:37 AM Mark Thompson <sw@jkqxz.net> wrote:

> On 05/10/18 21:45, Alex Sukhanov wrote:
> > On Mon, Oct 1, 2018 at 11:01 AM <alx.sukhanov@gmail.com> wrote:
> >
> >> From: Alex Sukhanov <asukhanov@google.com>
> >>
> >> ---
> >>  libavformat/ivfenc.c | 50 +++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 38 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
> >> index 66441a2a43..6410828533 100644
> >> --- a/libavformat/ivfenc.c
> >> +++ b/libavformat/ivfenc.c
> >> @@ -36,19 +36,29 @@ static int ivf_write_header(AVFormatContext *s)
> >>          return AVERROR(EINVAL);
> >>      }
> >>      par = s->streams[0]->codecpar;
> >> -    if (par->codec_type != AVMEDIA_TYPE_VIDEO ||
> >> -        !(par->codec_id == AV_CODEC_ID_AV1 ||
> >> -          par->codec_id == AV_CODEC_ID_VP8 ||
> >> -          par->codec_id == AV_CODEC_ID_VP9)) {
> >> -        av_log(s, AV_LOG_ERROR, "Currently only VP8, VP9 and AV1 are
> >> supported!\n");
> >> -        return AVERROR(EINVAL);
> >> -    }
> >>      avio_write(pb, "DKIF", 4);
> >>      avio_wl16(pb, 0); // version
> >>      avio_wl16(pb, 32); // header length
> >> -    avio_wl32(pb,
> >> -              par->codec_id == AV_CODEC_ID_VP9 ? AV_RL32("VP90") :
> >> -              par->codec_id == AV_CODEC_ID_VP8 ? AV_RL32("VP80") :
> >> AV_RL32("AV01"));
> >> +    switch (par->codec_id) {
> >> +      case AV_CODEC_ID_AV1:
> >> +        avio_wl32(pb, AV_RL32("AV01"));
> >> +        break;
> >> +      case AV_CODEC_ID_H264:
> >> +        avio_wl32(pb, AV_RL32("H264"));
> >> +        break;
> >> +      case AV_CODEC_ID_HEVC:
> >> +        avio_wl32(pb, AV_RL32("HEVC"));
> >> +        break;
> >> +      case AV_CODEC_ID_VP8:
> >> +        avio_wl32(pb, AV_RL32("VP80"));
> >> +        break;
> >> +      case AV_CODEC_ID_VP9:
> >> +        avio_wl32(pb, AV_RL32("VP90"));
> >> +        break;
> >> +      default:
> >> +        av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8
> and
> >> VP9 and AV1 are supported!\n");
> >> +        return AVERROR(EINVAL);
> >> +    }
> >>      avio_wl16(pb, par->width);
> >>      avio_wl16(pb, par->height);
> >>      avio_wl32(pb, s->streams[0]->time_base.den);
> >> @@ -95,16 +105,32 @@ static int ivf_check_bitstream(struct
> AVFormatContext
> >> *s, const AVPacket *pkt)
> >>      int ret = 1;
> >>      AVStream *st = s->streams[pkt->stream_index];
> >>
> >> -    if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
> >> +    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
> >> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
> >> +                             (AV_RB24(pkt->data) != 0x000001 ||
> >> +                              (st->codecpar->extradata_size > 0 &&
> >> +                               st->codecpar->extradata[0] == 1)))
> >> +            ret = ff_stream_add_bitstream_filter(st,
> "h264_mp4toannexb",
> >> NULL);
> >> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_HEVC) {
> >> +        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
> >> +                             (AV_RB24(pkt->data) != 0x000001 ||
> >> +                              (st->codecpar->extradata_size > 0 &&
> >> +                               st->codecpar->extradata[0] == 1)))
> >> +            ret = ff_stream_add_bitstream_filter(st,
> "hevc_mp4toannexb",
> >> NULL);
> >> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_VP9) {
> >>          ret = ff_stream_add_bitstream_filter(st, "vp9_superframe",
> NULL);
> >> +    }
> >>
> >>      return ret;
> >>  }
> >>
> >>  static const AVCodecTag codec_ivf_tags[] = {
> >> +    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
> >> +    { AV_CODEC_ID_H264, MKTAG('H', '2', '6', '4') },
> >> +    { AV_CODEC_ID_HEVC, MKTAG('H', 'E', 'V', 'C') },
> >>      { AV_CODEC_ID_VP8,  MKTAG('V', 'P', '8', '0') },
> >>      { AV_CODEC_ID_VP9,  MKTAG('V', 'P', '9', '0') },
> >> -    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
> >> +
> >>      { AV_CODEC_ID_NONE, 0 }
> >>  };
> >>
> >> --
> >> 2.19.0.605.g01d371f741-goog
> >>
> >>
> > Can you please take a look on this patch?
>
> Can you answer the question posed in <
> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-September/234655.html>
> about the reasoning for this patch?
>
> In particular, it might be helpful if you could point out any
> tools/standards which support (or are in future intending to support) this
> format - in my experience pretty much everything dealing with H.26[45] raw
> streams uses the Annex B format.
>
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Hi Mark,

at Google we have some old service which is still running and it works only
with the IVF container. It would be great if ffmpeg could generate such
videos so we could give them to the service then. Given that ffmpeg IVF
demuxer already supports reading such files, I think it's reasonable to
make IVF muxer be able to generate them.
Hope it answers the question.

Thank you
Jan Ekström Oct. 11, 2018, 9:21 p.m.
On Thu, Oct 11, 2018 at 10:58 PM Alex Sukhanov <alx.sukhanov@gmail.com> wrote:
>
> Hi Mark,
>
> at Google we have some old service which is still running and it works only
> with the IVF container. It would be great if ffmpeg could generate such
> videos so we could give them to the service then. Given that ffmpeg IVF
> demuxer already supports reading such files, I think it's reasonable to
> make IVF muxer be able to generate them.
> Hope it answers the question.
>
> Thank you

Given the amount of code is not large I'm not against having it in,
but if it's not something that ever was meant to go into the public
I'd probably disable creation of such files unless you enable a less
standards-compliant strictness mode.

Or if others are against it, I'm OK with this thing not being included
as it clearly is to enable some GOOG internal workflows only.

Best regards,
Jan
Derek Buitenhuis Oct. 11, 2018, 9:47 p.m.
On 11/10/2018 22:21, Jan Ekström wrote:
> I'd probably disable creation of such files unless you enable a less
> standards-compliant strictness mode.

Is there even an IVF spec, though? If not, there's not really such a
thing as "standards-compliant".

- Derek
alx.sukhanov@gmail.com Oct. 11, 2018, 10:39 p.m.
On Thu, Oct 11, 2018 at 2:47 PM Derek Buitenhuis <derek.buitenhuis@gmail.com>
wrote:

> On 11/10/2018 22:21, Jan Ekström wrote:
> > I'd probably disable creation of such files unless you enable a less
> > standards-compliant strictness mode.
>
> Is there even an IVF spec, though? If not, there's not really such a
> thing as "standards-compliant".
>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


The only "spec" I'm aware of: https://wiki.multimedia.cx/index.php/IVF
Derek Buitenhuis Oct. 12, 2018, 12:13 a.m.
On 11/10/2018 23:39, Alex Sukhanov wrote:
> The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF

Yeah, not really a spec.

I have no strong objections, I guess.

- Derek
Michael Niedermayer Oct. 14, 2018, 12:16 a.m.
On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> On 11/10/2018 23:39, Alex Sukhanov wrote:
> > The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> 
> Yeah, not really a spec.
> 
> I have no strong objections, I guess.

so IIUC noone is against this ?
if so i will apply the last patch in a few days unless i forget or
someone else does before

thx

[...]
Jan Ekström Oct. 14, 2018, 10:27 a.m.
On Sun, Oct 14, 2018 at 3:17 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> > On 11/10/2018 23:39, Alex Sukhanov wrote:
> > > The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> >
> > Yeah, not really a spec.
> >
> > I have no strong objections, I guess.
>
> so IIUC noone is against this ?
> if so i will apply the last patch in a few days unless i forget or
> someone else does before
>
> thx
>

As mentioned, since this is nothing should have ever been used outside
of one of GOOG's legacy systems, I would only apply this with a
warning and strictness requirement of experimental or whatever level
matches these sorts of cases.

Nothing in public that utilizes IVF files will output or read AVC/HEVC
from IVF files. It was explicitly only utilized by the webm/libvpx
project publicly, and that for obvious reasons will not support
AVC/HEVC.

Best regards,
Jan
Jan Ekström Oct. 14, 2018, 10:29 a.m.
On Sun, Oct 14, 2018 at 1:27 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Sun, Oct 14, 2018 at 3:17 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> > > On 11/10/2018 23:39, Alex Sukhanov wrote:
> > > > The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> > >
> > > Yeah, not really a spec.
> > >
> > > I have no strong objections, I guess.
> >
> > so IIUC noone is against this ?
> > if so i will apply the last patch in a few days unless i forget or
> > someone else does before
> >
> > thx
> >
>
> As mentioned, since this is nothing should have ever been used outside
> of one of GOOG's legacy systems, I would only apply this with a
> warning and strictness requirement of experimental or whatever level
> matches these sorts of cases.
>

For the record, this was regarding the muxer so that unknowing people
will not generate such files that nothing else can read.

Best regards,
Jan
Michael Niedermayer Oct. 14, 2018, 12:59 p.m.
On Sun, Oct 14, 2018 at 01:29:53PM +0300, Jan Ekström wrote:
> On Sun, Oct 14, 2018 at 1:27 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Sun, Oct 14, 2018 at 3:17 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> > > > On 11/10/2018 23:39, Alex Sukhanov wrote:
> > > > > The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> > > >
> > > > Yeah, not really a spec.
> > > >
> > > > I have no strong objections, I guess.
> > >
> > > so IIUC noone is against this ?
> > > if so i will apply the last patch in a few days unless i forget or
> > > someone else does before
> > >
> > > thx
> > >
> >
> > As mentioned, since this is nothing should have ever been used outside
> > of one of GOOG's legacy systems, I would only apply this with a
> > warning and strictness requirement of experimental or whatever level
> > matches these sorts of cases.
> >
> 
> For the record, this was regarding the muxer so that unknowing people
> will not generate such files that nothing else can read.

If you choose a random container and store a random codec in it (which you can do)
chances are theres not much that will play it.

for example i just tried muxing a stream of png images into mpeg and FFmpeg
does that without complaining.
./ffmpeg -i matrixbench_mpeg2.mpg -vcodec png test.mpeg

So if we do not check this for a major format that has a proper specification
which i belive nowhere allows png

Why should we add a limitation on a format that has nothing saying that you
cannot put some other codec into it ?
And i mean there IS software that uses that what is asked for here.

If you object because the text at https://wiki.multimedia.cx/index.php/IVF
does not list all codecs. Well its a wiki, the author of the patch could
get an account and update this. In fact independant of this discussion here
it surely wont hurt if the people using the format review and update that
page.

The other concern IIUC (please correct me if i misunderstood)
is that its a legacy format. FFmpeg is full of legacy format support, we suport
some really bizare formats ...

And another concern IIUC ( again please correct me if i misunderstood)
is that someone might unintentionally generate a unsupported file
this is tricky, the default video codec for ivf is AV_CODEC_ID_VP8
to get something else, one would need to manually override this and
as we already saw thats not something we current protect against very
well.

Iam not against disallowing the new codecs by default but it feels 
inconsistent compared to other containers.

what do you say ? did i convince you or should we disallow it and add
a check. Iam happy with whatever people agree on

thanks

[...]
James Almer Oct. 14, 2018, 1:24 p.m.
On 10/14/2018 9:59 AM, Michael Niedermayer wrote:
> On Sun, Oct 14, 2018 at 01:29:53PM +0300, Jan Ekström wrote:
>> On Sun, Oct 14, 2018 at 1:27 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>>
>>> On Sun, Oct 14, 2018 at 3:17 AM Michael Niedermayer
>>> <michael@niedermayer.cc> wrote:
>>>>
>>>> On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
>>>>> On 11/10/2018 23:39, Alex Sukhanov wrote:
>>>>>> The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
>>>>>
>>>>> Yeah, not really a spec.
>>>>>
>>>>> I have no strong objections, I guess.
>>>>
>>>> so IIUC noone is against this ?
>>>> if so i will apply the last patch in a few days unless i forget or
>>>> someone else does before
>>>>
>>>> thx
>>>>
>>>
>>> As mentioned, since this is nothing should have ever been used outside
>>> of one of GOOG's legacy systems, I would only apply this with a
>>> warning and strictness requirement of experimental or whatever level
>>> matches these sorts of cases.
>>>
>>
>> For the record, this was regarding the muxer so that unknowing people
>> will not generate such files that nothing else can read.
> 
> If you choose a random container and store a random codec in it (which you can do)
> chances are theres not much that will play it.
> 
> for example i just tried muxing a stream of png images into mpeg and FFmpeg
> does that without complaining.
> ./ffmpeg -i matrixbench_mpeg2.mpg -vcodec png test.mpeg
> 
> So if we do not check this for a major format that has a proper specification
> which i belive nowhere allows png
> 
> Why should we add a limitation on a format that has nothing saying that you
> cannot put some other codec into it ?

The proper thing to do would be to effectively disallow such invalid
muxing scenarios, fixing this instead of adding even more wrong cases to
the pile.
I'm fairly sure we blocked a patch to allow muxing hevc into flv years
ago. That's what needs to be done in general.
Michael Niedermayer Oct. 15, 2018, 9:14 a.m.
On Sun, Oct 14, 2018 at 10:24:22AM -0300, James Almer wrote:
> On 10/14/2018 9:59 AM, Michael Niedermayer wrote:
> > On Sun, Oct 14, 2018 at 01:29:53PM +0300, Jan Ekström wrote:
> >> On Sun, Oct 14, 2018 at 1:27 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >>>
> >>> On Sun, Oct 14, 2018 at 3:17 AM Michael Niedermayer
> >>> <michael@niedermayer.cc> wrote:
> >>>>
> >>>> On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> >>>>> On 11/10/2018 23:39, Alex Sukhanov wrote:
> >>>>>> The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> >>>>>
> >>>>> Yeah, not really a spec.
> >>>>>
> >>>>> I have no strong objections, I guess.
> >>>>
> >>>> so IIUC noone is against this ?
> >>>> if so i will apply the last patch in a few days unless i forget or
> >>>> someone else does before
> >>>>
> >>>> thx
> >>>>
> >>>
> >>> As mentioned, since this is nothing should have ever been used outside
> >>> of one of GOOG's legacy systems, I would only apply this with a
> >>> warning and strictness requirement of experimental or whatever level
> >>> matches these sorts of cases.
> >>>
> >>
> >> For the record, this was regarding the muxer so that unknowing people
> >> will not generate such files that nothing else can read.
> > 
> > If you choose a random container and store a random codec in it (which you can do)
> > chances are theres not much that will play it.
> > 
> > for example i just tried muxing a stream of png images into mpeg and FFmpeg
> > does that without complaining.
> > ./ffmpeg -i matrixbench_mpeg2.mpg -vcodec png test.mpeg
> > 
> > So if we do not check this for a major format that has a proper specification
> > which i belive nowhere allows png
> > 
> > Why should we add a limitation on a format that has nothing saying that you
> > cannot put some other codec into it ?
> 
> The proper thing to do would be to effectively disallow such invalid
> muxing scenarios, fixing this instead of adding even more wrong cases to
> the pile.
> I'm fairly sure we blocked a patch to allow muxing hevc into flv years
> ago. That's what needs to be done in general.

I think where we disagree is not that "we should disallow invalid muxing"
but if this is invalid/wrong. And also it seems to me that people appear to
be very quick in jumping onto such "lets block it" movments without doing 
much research

about new codecs in IVF, which this thread is about, why is that invalid ?

our ivf demuxer refers to itself as "On2 IVF" so i presume the format was 
created by On2
(googling for On2 IVF results in only strange results interrestingly)
but presumably IVF was created by On2
In February 2010, On2 Technologies was acquired by Google (https://en.wikipedia.org/wiki/On2_Technologies)

And google is using IVF apparently from what was said in this thread with more
codecs. Well from how i understand it (please correct me if iam wrong)
IVF is now a format "owned" by google. They certainly can add things to
it. Theres nothing wrong or invalid on this.

Thanks

[...]
Vittorio Giovara Oct. 15, 2018, 4:28 p.m.
On Thu, Oct 11, 2018 at 5:28 PM Jan Ekström <jeebjp@gmail.com> wrote:

> On Thu, Oct 11, 2018 at 10:58 PM Alex Sukhanov <alx.sukhanov@gmail.com>
> wrote:
> >
> > Hi Mark,
> >
> > at Google we have some old service which is still running and it works
> only
> > with the IVF container. It would be great if ffmpeg could generate such
> > videos so we could give them to the service then. Given that ffmpeg IVF
> > demuxer already supports reading such files, I think it's reasonable to
> > make IVF muxer be able to generate them.
> > Hope it answers the question.
> >
> > Thank you
>
> Given the amount of code is not large I'm not against having it in,
> but if it's not something that ever was meant to go into the public
> I'd probably disable creation of such files unless you enable a less
> standards-compliant strictness mode.
>

maybe creation of such files could be allowed only with a strict compliance
flag?
alx.sukhanov@gmail.com Oct. 18, 2018, 6:17 p.m.
On Mon, Oct 15, 2018 at 9:35 AM Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Thu, Oct 11, 2018 at 5:28 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> > On Thu, Oct 11, 2018 at 10:58 PM Alex Sukhanov <alx.sukhanov@gmail.com>
> > wrote:
> > >
> > > Hi Mark,
> > >
> > > at Google we have some old service which is still running and it works
> > only
> > > with the IVF container. It would be great if ffmpeg could generate such
> > > videos so we could give them to the service then. Given that ffmpeg IVF
> > > demuxer already supports reading such files, I think it's reasonable to
> > > make IVF muxer be able to generate them.
> > > Hope it answers the question.
> > >
> > > Thank you
> >
> > Given the amount of code is not large I'm not against having it in,
> > but if it's not something that ever was meant to go into the public
> > I'd probably disable creation of such files unless you enable a less
> > standards-compliant strictness mode.
> >
>
> maybe creation of such files could be allowed only with a strict compliance
> flag?
> --
> Vittorio
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Vittorio,

can you please elaborate your proposal? Is there already a flag in ffmpeg
that we can consider use for that? What would be a benefit of guarding
h264/hevc muxing into the IVF container by the flag?
Thank you
alx.sukhanov@gmail.com Oct. 24, 2018, 11:50 p.m.
On Sat, Oct 13, 2018 at 5:17 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> > On 11/10/2018 23:39, Alex Sukhanov wrote:
> > > The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> >
> > Yeah, not really a spec.
> >
> > I have no strong objections, I guess.
>
> so IIUC noone is against this ?
> if so i will apply the last patch in a few days unless i forget or
> someone else does before
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Everything should be made as simple as possible, but not simpler.
> -- Albert Einstein
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Michael, can you please apply this patch?
Michael Niedermayer Oct. 25, 2018, 11:46 p.m.
On Wed, Oct 24, 2018 at 04:50:07PM -0700, Alex Sukhanov wrote:
> On Sat, Oct 13, 2018 at 5:17 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Fri, Oct 12, 2018 at 01:13:45AM +0100, Derek Buitenhuis wrote:
> > > On 11/10/2018 23:39, Alex Sukhanov wrote:
> > > > The only "spec" I'm aware of:https://wiki.multimedia.cx/index.php/IVF
> > >
> > > Yeah, not really a spec.
> > >
> > > I have no strong objections, I guess.
> >
> > so IIUC noone is against this ?
> > if so i will apply the last patch in a few days unless i forget or
> > someone else does before
> >
> > thx
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Everything should be made as simple as possible, but not simpler.
> > -- Albert Einstein
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> Michael, can you please apply this patch?

It seemed to me that jan, james and vittorio where not happy about it
It would be better if everyone agreed with what is applied, maybe you
can talk with them to see if you can find something that everyone is
happy with.

Thanks

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c
index 66441a2a43..6410828533 100644
--- a/libavformat/ivfenc.c
+++ b/libavformat/ivfenc.c
@@ -36,19 +36,29 @@  static int ivf_write_header(AVFormatContext *s)
         return AVERROR(EINVAL);
     }
     par = s->streams[0]->codecpar;
-    if (par->codec_type != AVMEDIA_TYPE_VIDEO ||
-        !(par->codec_id == AV_CODEC_ID_AV1 ||
-          par->codec_id == AV_CODEC_ID_VP8 ||
-          par->codec_id == AV_CODEC_ID_VP9)) {
-        av_log(s, AV_LOG_ERROR, "Currently only VP8, VP9 and AV1 are supported!\n");
-        return AVERROR(EINVAL);
-    }
     avio_write(pb, "DKIF", 4);
     avio_wl16(pb, 0); // version
     avio_wl16(pb, 32); // header length
-    avio_wl32(pb,
-              par->codec_id == AV_CODEC_ID_VP9 ? AV_RL32("VP90") :
-              par->codec_id == AV_CODEC_ID_VP8 ? AV_RL32("VP80") : AV_RL32("AV01"));
+    switch (par->codec_id) {
+      case AV_CODEC_ID_AV1:
+        avio_wl32(pb, AV_RL32("AV01"));
+        break;
+      case AV_CODEC_ID_H264:
+        avio_wl32(pb, AV_RL32("H264"));
+        break;
+      case AV_CODEC_ID_HEVC:
+        avio_wl32(pb, AV_RL32("HEVC"));
+        break;
+      case AV_CODEC_ID_VP8:
+        avio_wl32(pb, AV_RL32("VP80"));
+        break;
+      case AV_CODEC_ID_VP9:
+        avio_wl32(pb, AV_RL32("VP90"));
+        break;
+      default:
+        av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and VP9 and AV1 are supported!\n");
+        return AVERROR(EINVAL);
+    }
     avio_wl16(pb, par->width);
     avio_wl16(pb, par->height);
     avio_wl32(pb, s->streams[0]->time_base.den);
@@ -95,16 +105,32 @@  static int ivf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
     int ret = 1;
     AVStream *st = s->streams[pkt->stream_index];
 
-    if (st->codecpar->codec_id == AV_CODEC_ID_VP9)
+    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
+        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
+                             (AV_RB24(pkt->data) != 0x000001 ||
+                              (st->codecpar->extradata_size > 0 &&
+                               st->codecpar->extradata[0] == 1)))
+            ret = ff_stream_add_bitstream_filter(st, "h264_mp4toannexb", NULL);
+    } else if (st->codecpar->codec_id == AV_CODEC_ID_HEVC) {
+        if (pkt->size >= 5 && AV_RB32(pkt->data) != 0x0000001 &&
+                             (AV_RB24(pkt->data) != 0x000001 ||
+                              (st->codecpar->extradata_size > 0 &&
+                               st->codecpar->extradata[0] == 1)))
+            ret = ff_stream_add_bitstream_filter(st, "hevc_mp4toannexb", NULL);
+    } else if (st->codecpar->codec_id == AV_CODEC_ID_VP9) {
         ret = ff_stream_add_bitstream_filter(st, "vp9_superframe", NULL);
+    }
 
     return ret;
 }
 
 static const AVCodecTag codec_ivf_tags[] = {
+    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
+    { AV_CODEC_ID_H264, MKTAG('H', '2', '6', '4') },
+    { AV_CODEC_ID_HEVC, MKTAG('H', 'E', 'V', 'C') },
     { AV_CODEC_ID_VP8,  MKTAG('V', 'P', '8', '0') },
     { AV_CODEC_ID_VP9,  MKTAG('V', 'P', '9', '0') },
-    { AV_CODEC_ID_AV1,  MKTAG('A', 'V', '0', '1') },
+
     { AV_CODEC_ID_NONE, 0 }
 };