| Message ID | 20180928185110.212678-1-alx.sukhanov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
2018-09-28 20:51 GMT+02:00, alx.sukhanov@gmail.com <alx.sukhanov@gmail.com>: > From: Alex Sukhanov <asukhanov@google.com> > if (par->codec_type != AVMEDIA_TYPE_VIDEO || > !(par->codec_id == AV_CODEC_ID_AV1 || > + par->codec_id == AV_CODEC_ID_H264 || > + par->codec_id == AV_CODEC_ID_HEVC || > 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"); > + av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and > 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: > + break; Please either remove this or make it an assert(). Carl Eugen
On 9/30/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-09-28 20:51 GMT+02:00, alx.sukhanov@gmail.com <alx.sukhanov@gmail.com>: >> From: Alex Sukhanov <asukhanov@google.com> > >> if (par->codec_type != AVMEDIA_TYPE_VIDEO || >> !(par->codec_id == AV_CODEC_ID_AV1 || >> + par->codec_id == AV_CODEC_ID_H264 || >> + par->codec_id == AV_CODEC_ID_HEVC || >> 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"); >> + av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and >> 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: >> + break; > > Please either remove this or make it an assert(). No, AVERROR_BUG is better.
Paul B Mahol (2018-09-30):
> No, AVERROR_BUG is better.
For consistency within a single function, an assert is the correct
choice.
Regards,
On 9/28/2018 3:51 PM, alx.sukhanov@gmail.com wrote: > From: Alex Sukhanov <asukhanov@google.com> > > --- > libavformat/ivfenc.c | 41 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 5 deletions(-) > > diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c > index 66441a2a43..68c36daf09 100644 > --- a/libavformat/ivfenc.c > +++ b/libavformat/ivfenc.c > @@ -38,17 +38,35 @@ static int ivf_write_header(AVFormatContext *s) > 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_H264 || > + par->codec_id == AV_CODEC_ID_HEVC || > 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"); > + av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and 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: > + break; > + } > avio_wl16(pb, par->width); > avio_wl16(pb, par->height); > avio_wl32(pb, s->streams[0]->time_base.den); > @@ -95,8 +113,21 @@ 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; > } > This is missing adding the new codec tags to codec_ivf_tags[] at the end of the file.
On 9/30/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-09-30): >> No, AVERROR_BUG is better. > > For consistency within a single function, an assert is the correct > choice. No, it is not. Asserts do not add any consistency.
Paul B Mahol (2018-09-30): > > For consistency within a single function, an assert is the correct > > choice. > No, it is not. Asserts do not add any consistency. Asserts do not add consistency indeed. Their purpose is to check consistency, which is exactly what needs testing here: the consistency between the preliminary test and the later switch statement. If you want to argue for AVERROR_BUG, please give arguments. Regards,
Sn Sun, Sep 30, 2018 at 8:56 PM <alx.sukhanov@gmail.com> wrote: > > ... > + 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); Please verify that there is no AVCc/Annex B deciding helper function within lavf/lavc that could be used here. I tried to ask about this on IRC but didn't get responses so I can't straight up note you a function name. If there's none, then it might be worth making a lavf helper for that since I think at least movenc/matroskaenc try to figure out if the input bit stream is Annex B and convert it the other way. Additionally, please make sure that the files created with these changes can be demuxed with the FFmpeg IVF demuxer (ivfdec.c). I would guess the H.264 stuff would work, but since the IVF demuxer utilizes the ff_codec_bmp_tags list, which doesn't seem to contain HEVC entries I would guess additional stuff would be required there. That of course doesn't have to be within this patch, but just within this patch set (one patch for demuxer, one for muxer). Best regards, Jan
On Mon, Oct 1, 2018 at 12:01 AM Jan Ekström <jeebjp@gmail.com> wrote: > > Additionally, please make sure that the files created with these > changes can be demuxed with the FFmpeg IVF demuxer (ivfdec.c). I would > guess the H.264 stuff would work, but since the IVF demuxer utilizes > the ff_codec_bmp_tags list, which doesn't seem to contain HEVC entries > I would guess additional stuff would be required there. > For the record, I was noted that people were previously against adding HEVC to riff.c's lists as that would end up supporting HEVC in AVI, which we would prefer not to see in the wild. Thus I think the proper way of doing this would be to have the IVF demuxer to have its own AVCodecTag list instead of using the global RIFF list. This could be shared between the demuxer and muxer so it doesn't get duplicated. Best regards, Jan
On 9/30/2018 6:01 PM, Jan Ekström wrote: > Sn Sun, Sep 30, 2018 at 8:56 PM <alx.sukhanov@gmail.com> wrote: >> >> ... >> + 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); > > Please verify that there is no AVCc/Annex B deciding helper function > within lavf/lavc that could be used here. I tried to ask about this on > IRC but didn't get responses so I can't straight up note you a > function name. If there's none, then it might be worth making a lavf > helper for that since I think at least movenc/matroskaenc try to > figure out if the input bit stream is Annex B and convert it the other > way. The *_mp4toannexb bitstream filters convert mp4 encapsulated packets into annex-b, or pass already annex-b formatted packets through. Assuming ivf is meant to use annex-b, then what this patch is doing is correct. > > Additionally, please make sure that the files created with these > changes can be demuxed with the FFmpeg IVF demuxer (ivfdec.c). I would > guess the H.264 stuff would work, but since the IVF demuxer utilizes > the ff_codec_bmp_tags list, which doesn't seem to contain HEVC entries > I would guess additional stuff would be required there. > > That of course doesn't have to be within this patch, but just within > this patch set (one patch for demuxer, one for muxer). Yes, a demuxer patch removing the usage of ff_codec_bmp_tags and replacing it with a custom ivf specific one is needed after, or probably before this patch. > > Best regards, > Jan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 9/30/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-09-30): >> > For consistency within a single function, an assert is the correct >> > choice. >> No, it is not. Asserts do not add any consistency. > > Asserts do not add consistency indeed. Their purpose is to check > consistency, which is exactly what needs testing here: the consistency > between the preliminary test and the later switch statement. > > If you want to argue for AVERROR_BUG, please give arguments. Same is given with AVERROR_BUG, instead of hard crashing with assert, which is very unprofessional.
Paul B Mahol (2018-10-01): > Same is given with AVERROR_BUG, instead of hard crashing with assert, which is > very unprofessional. A program crashing seems very professional to me. The most professional program ever, windows, has a long history of crashing. Fortunately, FFmpeg is not a professional project, we can afford to do things properly. A program that crashes is sloppy, indeed. Assert failures are bad. But AVERROR_BUG is just as bad. It should not happen after commit either. Therefore, this is not an argument to choose one over the other. Regards,
On 10/1/18, Nicolas George <george@nsup.org> wrote: > Paul B Mahol (2018-10-01): >> Same is given with AVERROR_BUG, instead of hard crashing with assert, >> which is >> very unprofessional. > > A program crashing seems very professional to me. The most professional > program ever, windows, has a long history of crashing. Fortunately, > FFmpeg is not a professional project, we can afford to do things > properly. > > A program that crashes is sloppy, indeed. Assert failures are bad. But > AVERROR_BUG is just as bad. It should not happen after commit either. > Therefore, this is not an argument to choose one over the other. I lost motivation to feed more.
On Sun, Sep 30, 2018 at 2:09 PM Jan Ekström <jeebjp@gmail.com> wrote: > Sn Sun, Sep 30, 2018 at 8:56 PM <alx.sukhanov@gmail.com> wrote: > > > > ... > > + 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); > > Please verify that there is no AVCc/Annex B deciding helper function > within lavf/lavc that could be used here. I tried to ask about this on > IRC but didn't get responses so I can't straight up note you a > function name. If there's none, then it might be worth making a lavf > helper for that since I think at least movenc/matroskaenc try to > figure out if the input bit stream is Annex B and convert it the other > way. > > Additionally, please make sure that the files created with these > changes can be demuxed with the FFmpeg IVF demuxer (ivfdec.c). I would > guess the H.264 stuff would work, but since the IVF demuxer utilizes > the ff_codec_bmp_tags list, which doesn't seem to contain HEVC entries > I would guess additional stuff would be required there. > > Verified that demuxer can handle the files by running ffplay. Thank you. > That of course doesn't have to be within this patch, but just within > this patch set (one patch for demuxer, one for muxer). > > Best regards, > Jan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c index 66441a2a43..68c36daf09 100644 --- a/libavformat/ivfenc.c +++ b/libavformat/ivfenc.c @@ -38,17 +38,35 @@ static int ivf_write_header(AVFormatContext *s) 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_H264 || + par->codec_id == AV_CODEC_ID_HEVC || 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"); + av_log(s, AV_LOG_ERROR, "Currently only AV1, H264, HEVC, VP8 and 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: + break; + } avio_wl16(pb, par->width); avio_wl16(pb, par->height); avio_wl32(pb, s->streams[0]->time_base.den); @@ -95,8 +113,21 @@ 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; }
From: Alex Sukhanov <asukhanov@google.com> --- libavformat/ivfenc.c | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-)