Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: rename avif fields to heif
On Tue, Jan 9, 2024 at 11:56 AM James Almer wrote:
>
> They are no longer avif specific.
>
> Signed-off-by: James Almer
> ---
> libavformat/isom.h | 4 ++--
> libavformat/mov.c | 28 ++--
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index b30b9da65e..90c4fb5530 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -325,8 +325,8 @@ typedef struct MOVContext {
> int item_id;
> int extent_length;
> int64_t extent_offset;
> -} *avif_info;
> -int avif_info_size;
> +} *heif_info;
> +int heif_info_size;
> int64_t hvcC_offset;
> int hvcC_size;
> int interleaved_read;
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index c6398d6d81..12e82c66a9 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4916,15 +4916,15 @@ static int mov_read_custom(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> return ret;
> }
>
> -static int avif_add_stream(MOVContext *c, int item_id)
> +static int heif_add_stream(MOVContext *c, int item_id)
> {
> MOVStreamContext *sc;
> AVStream *st;
> int item_index = -1;
> if (c->fc->nb_streams)
> return AVERROR_INVALIDDATA;
> -for (int i = 0; i < c->avif_info_size; i++)
> -if (c->avif_info[i].item_id == item_id) {
> +for (int i = 0; i < c->heif_info_size; i++)
> +if (c->heif_info[i].item_id == item_id) {
> item_index = i;
> break;
> }
> @@ -4987,8 +4987,8 @@ static int avif_add_stream(MOVContext *c, int item_id)
> sc->stts_data[0].count = 1;
> // Not used for still images. But needed by mov_build_index.
> sc->stts_data[0].duration = 0;
> -sc->sample_sizes[0] = c->avif_info[item_index].extent_length;
> -sc->chunk_offsets[0] = c->avif_info[item_index].extent_offset;
> +sc->sample_sizes[0] = c->heif_info[item_index].extent_length;
> +sc->chunk_offsets[0] = c->heif_info[item_index].extent_offset;
>
> mov_build_index(c, st);
> return 0;
> @@ -5013,7 +5013,7 @@ static int mov_read_meta(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> if (c->is_still_picture_avif) {
> int ret;
> // Add a stream for the YUV planes (primary item).
> -if ((ret = avif_add_stream(c, c->primary_item_id)) < 0)
> +if ((ret = heif_add_stream(c, c->primary_item_id)) < 0)
> return ret;
> // For still AVIF images, the meta box contains all the
> // necessary information that would generally be provided by
> the
> @@ -7820,7 +7820,7 @@ static int mov_read_iloc(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> return 0;
> }
>
> -if (c->avif_info) {
> +if (c->heif_info) {
> av_log(c->fc, AV_LOG_INFO, "Duplicate iloc box found\n");
> return 0;
> }
> @@ -7841,16 +7841,16 @@ static int mov_read_iloc(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> }
> item_count = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
>
> -c->avif_info = av_malloc_array(item_count, sizeof(*c->avif_info));
> -if (!c->avif_info)
> +c->heif_info = av_malloc_array(item_count, sizeof(*c->heif_info));
> +if (!c->heif_info)
> return AVERROR(ENOMEM);
> -c->avif_info_size = item_count;
> +c->heif_info_size = item_count;
>
> for (int i = 0; i < item_count; i++) {
> int item_id = (version < 2) ? avio_rb16(pb) : avio_rb32(pb);
> if (avio_feof(pb))
> return AVERROR_INVALIDDATA;
> -c->avif_info[i].item_id = item_id;
> +c->heif_info[i].item_id = item_id;
>
> if (version > 0)
> avio_rb16(pb); // construction_method.
> @@ -7867,8 +7867,8 @@ static int mov_read_iloc(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> if (rb_size(pb, _offset, offset_size) < 0 ||
> rb_size(pb, _length, length_size) < 0)
> return AVERROR_INVALIDDATA;
> -c->avif_info[i].extent_length = extent_length;
> -c->avif_info[i].extent_offset = base_offset + extent_offset;
> +c->heif_info[i].extent_length = extent_length;
> +c->heif_info[i].extent_offset = base_offset + extent_offset;
> }
> }
>
> @@ -8502,7 +8502,7 @@ static int mov_read_close(AVFormatContext *s)
>
> av_freep(>aes_decrypt);
> av_freep(>chapter_tracks);
> -av_freep(>avif_info);
> +av_freep(>heif_info);
>
> return 0;
> }
> --
> 2.43.0
>
> ___
> 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".
lgtm
--
Vignesh
___
ffmpeg-devel mailing list
Re: [FFmpeg-devel] [PATCH v3] avformat/mov: Add support for demuxing still HEIC images
On Tue, Jan 9, 2024 at 4:39 AM James Almer wrote: > > On 10/4/2023 1:40 PM, Vignesh Venkatasubramanian via ffmpeg-devel wrote: > > They are similar to AVIF images (both use the HEIF container). > > The only additional work needed is to parse the hvcC box and put > > it in the extradata. > > > > With this patch applied, ffmpeg (when built with an HEVC decoder) > > is able to decode the files in > > https://github.com/nokiatech/heif/tree/gh-pages/content/images > > > > Also add a couple of fate tests with samples from > > https://github.com/nokiatech/heif_conformance/tree/master/conformance_files > > > > Partially fixes trac ticket #6521. > > > > Signed-off-by: Vignesh Venkatasubramanian > > --- > > libavformat/isom.h| 2 + > > libavformat/mov.c | 41 ++- > > tests/fate/mov.mak| 6 +++ > > .../fate/mov-heic-demux-still-image-1-item| 11 + > > .../mov-heic-demux-still-image-multiple-items | 11 + > > 5 files changed, 70 insertions(+), 1 deletion(-) > > create mode 100644 tests/ref/fate/mov-heic-demux-still-image-1-item > > create mode 100644 > > tests/ref/fate/mov-heic-demux-still-image-multiple-items > > This seems to have been forgotten, so I'll apply it soon. Sorry that was my bad. Thanks for applying. > ___ > 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". -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH v3] avformat/mov: Add support for demuxing still HEIC images
On Thu, Oct 5, 2023 at 3:40 PM Vignesh Venkat wrote:
>
>
>
> On Thu, Oct 5, 2023 at 10:36 AM Vittorio Giovara
> wrote:
>>
>>
>>
>> On Wed, Oct 4, 2023 at 12:40 PM Vignesh Venkatasubramanian via ffmpeg-devel
>> wrote:
>>>
>>> They are similar to AVIF images (both use the HEIF container).
>>> The only additional work needed is to parse the hvcC box and put
>>> it in the extradata.
>>>
>>> With this patch applied, ffmpeg (when built with an HEVC decoder)
>>> is able to decode the files in
>>> https://github.com/nokiatech/heif/tree/gh-pages/content/images
>>>
>>> Also add a couple of fate tests with samples from
>>> https://github.com/nokiatech/heif_conformance/tree/master/conformance_files
>>>
>>> Partially fixes trac ticket #6521.
>>>
>>> Signed-off-by: Vignesh Venkatasubramanian
>>> ---
>>> libavformat/isom.h| 2 +
>>> libavformat/mov.c | 41 ++-
>>> tests/fate/mov.mak| 6 +++
>>> .../fate/mov-heic-demux-still-image-1-item| 11 +
>>> .../mov-heic-demux-still-image-multiple-items | 11 +
>>> 5 files changed, 70 insertions(+), 1 deletion(-)
>>> create mode 100644 tests/ref/fate/mov-heic-demux-still-image-1-item
>>> create mode 100644 tests/ref/fate/mov-heic-demux-still-image-multiple-items
>>>
>>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>>> index 3d375d7a46..b30b9da65e 100644
>>> --- a/libavformat/isom.h
>>> +++ b/libavformat/isom.h
>>> @@ -327,6 +327,8 @@ typedef struct MOVContext {
>>> int64_t extent_offset;
>>> } *avif_info;
>>> int avif_info_size;
>>> +int64_t hvcC_offset;
>>> +int hvcC_size;
>>> int interleaved_read;
>>> } MOVContext;
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 294c864fbd..d3747022bd 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -1218,7 +1218,8 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext
>>> *pb, MOVAtom atom)
>>> c->isom = 1;
>>> av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand: %.4s\n",(char
>>> *));
>>> av_dict_set(>fc->metadata, "major_brand", type, 0);
>>> -c->is_still_picture_avif = !strncmp(type, "avif", 4);
>>> +c->is_still_picture_avif = !strncmp(type, "avif", 4) ||
>>> + !strncmp(type, "mif1", 4);
>>> minor_ver = avio_rb32(pb); /* minor version */
>>> av_dict_set_int(>fc->metadata, "minor_version", minor_ver, 0);
>>>
>>> @@ -4911,6 +4912,19 @@ static int avif_add_stream(MOVContext *c, int
>>> item_id)
>>> st->priv_data = sc;
>>> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>>> st->codecpar->codec_id = AV_CODEC_ID_AV1;
>>> +if (c->hvcC_offset >= 0) {
>>> +int ret;
>>> +int64_t pos = avio_tell(c->fc->pb);
>>> +st->codecpar->codec_id = AV_CODEC_ID_HEVC;
>>> +if (avio_seek(c->fc->pb, c->hvcC_offset, SEEK_SET) !=
>>> c->hvcC_offset) {
>>> +av_log(c->fc, AV_LOG_ERROR, "Failed to seek to hvcC data.\n");
>>> +return AVERROR_UNKNOWN;
>>> +}
>>> +ret = ff_get_extradata(c->fc, st->codecpar, c->fc->pb,
>>> c->hvcC_size);
>>> +if (ret < 0)
>>> +return ret;
>>> +avio_seek(c->fc->pb, pos, SEEK_SET);
>>> +}
>>> sc->ffindex = st->index;
>>> c->trak_index = st->index;
>>> st->avg_frame_rate.num = st->avg_frame_rate.den = 1;
>>> @@ -4953,6 +4967,8 @@ static int avif_add_stream(MOVContext *c, int item_id)
>>>
>>> static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>> {
>>> +c->hvcC_offset = -1;
>>> +c->hvcC_size = 0;
>>> while (atom.size > 8) {
>>> uint32_t tag;
>>> if (avio_feof(pb))
>>> @@ -7827,6 +7843,28 @@ static int mov_read_iloc(MOVContext *c, AVIOContext
>>> *pb, MOVAtom atom)
>>> return atom.size;
>>> }
>>>
>>> +static int mov_rea
Re: [FFmpeg-devel] [PATCH v3] avformat/mov: Add support for demuxing still HEIC images
On Thu, Oct 5, 2023 at 10:36 AM Vittorio Giovara
wrote:
>
>
> On Wed, Oct 4, 2023 at 12:40 PM Vignesh Venkatasubramanian via
> ffmpeg-devel wrote:
>
>> They are similar to AVIF images (both use the HEIF container).
>> The only additional work needed is to parse the hvcC box and put
>> it in the extradata.
>>
>> With this patch applied, ffmpeg (when built with an HEVC decoder)
>> is able to decode the files in
>> https://github.com/nokiatech/heif/tree/gh-pages/content/images
>>
>> Also add a couple of fate tests with samples from
>>
>> https://github.com/nokiatech/heif_conformance/tree/master/conformance_files
>>
>> Partially fixes trac ticket #6521.
>>
>> Signed-off-by: Vignesh Venkatasubramanian
>> ---
>> libavformat/isom.h| 2 +
>> libavformat/mov.c | 41 ++-
>> tests/fate/mov.mak| 6 +++
>> .../fate/mov-heic-demux-still-image-1-item| 11 +
>> .../mov-heic-demux-still-image-multiple-items | 11 +
>> 5 files changed, 70 insertions(+), 1 deletion(-)
>> create mode 100644 tests/ref/fate/mov-heic-demux-still-image-1-item
>> create mode 100644
>> tests/ref/fate/mov-heic-demux-still-image-multiple-items
>>
>> diff --git a/libavformat/isom.h b/libavformat/isom.h
>> index 3d375d7a46..b30b9da65e 100644
>> --- a/libavformat/isom.h
>> +++ b/libavformat/isom.h
>> @@ -327,6 +327,8 @@ typedef struct MOVContext {
>> int64_t extent_offset;
>> } *avif_info;
>> int avif_info_size;
>> +int64_t hvcC_offset;
>> +int hvcC_size;
>> int interleaved_read;
>> } MOVContext;
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 294c864fbd..d3747022bd 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -1218,7 +1218,8 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext
>> *pb, MOVAtom atom)
>> c->isom = 1;
>> av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand:
>> %.4s\n",(char *));
>> av_dict_set(>fc->metadata, "major_brand", type, 0);
>> -c->is_still_picture_avif = !strncmp(type, "avif", 4);
>> +c->is_still_picture_avif = !strncmp(type, "avif", 4) ||
>> + !strncmp(type, "mif1", 4);
>> minor_ver = avio_rb32(pb); /* minor version */
>> av_dict_set_int(>fc->metadata, "minor_version", minor_ver, 0);
>>
>> @@ -4911,6 +4912,19 @@ static int avif_add_stream(MOVContext *c, int
>> item_id)
>> st->priv_data = sc;
>> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>> st->codecpar->codec_id = AV_CODEC_ID_AV1;
>> +if (c->hvcC_offset >= 0) {
>> +int ret;
>> +int64_t pos = avio_tell(c->fc->pb);
>> +st->codecpar->codec_id = AV_CODEC_ID_HEVC;
>> +if (avio_seek(c->fc->pb, c->hvcC_offset, SEEK_SET) !=
>> c->hvcC_offset) {
>> +av_log(c->fc, AV_LOG_ERROR, "Failed to seek to hvcC
>> data.\n");
>> +return AVERROR_UNKNOWN;
>> +}
>> +ret = ff_get_extradata(c->fc, st->codecpar, c->fc->pb,
>> c->hvcC_size);
>> +if (ret < 0)
>> +return ret;
>> +avio_seek(c->fc->pb, pos, SEEK_SET);
>> +}
>> sc->ffindex = st->index;
>> c->trak_index = st->index;
>> st->avg_frame_rate.num = st->avg_frame_rate.den = 1;
>> @@ -4953,6 +4967,8 @@ static int avif_add_stream(MOVContext *c, int
>> item_id)
>>
>> static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> {
>> +c->hvcC_offset = -1;
>> +c->hvcC_size = 0;
>> while (atom.size > 8) {
>> uint32_t tag;
>> if (avio_feof(pb))
>> @@ -7827,6 +7843,28 @@ static int mov_read_iloc(MOVContext *c,
>> AVIOContext *pb, MOVAtom atom)
>> return atom.size;
>> }
>>
>> +static int mov_read_iprp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>> +{
>> +int size = avio_rb32(pb);
>> +if (avio_rl32(pb) != MKTAG('i','p','c','o'))
>> +return AVERROR_INVALIDDATA;
>> +size -= 8;
>> +while (size > 0) {
>> +int sub_size, sub_type;
>> +sub_size = avio_rb32(pb);
>> +sub_type = avio_rl32(pb);
>> +sub_size -= 8;
>> +size -= sub_size + 8;
>> +if (sub_type == MKTAG('h','v','c','C')) {
>> +c->hvcC_offset = avio_tell(pb);
>> +c->hvcC_size = sub_size;
>> +break;
>> +}
>> +avio_skip(pb, sub_size);
>> +}
>> +return atom.size;
>> +}
>> +
>> static const MOVParseTableEntry mov_default_parse_table[] = {
>> { MKTAG('A','C','L','R'), mov_read_aclr },
>> { MKTAG('A','P','R','G'), mov_read_avid },
>> @@ -7934,6 +7972,7 @@ static const MOVParseTableEntry
>> mov_default_parse_table[] = {
>> { MKTAG('p','c','m','C'), mov_read_pcmc }, /* PCM configuration box */
>> { MKTAG('p','i','t','m'), mov_read_pitm },
>> { MKTAG('e','v','c','C'), mov_read_glbl },
>> +{ MKTAG('i','p','r','p'), mov_read_iprp },
>> { 0, NULL }
>> };
>>
>> diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
>> index
Re: [FFmpeg-devel] [PATCH] avformat/mov: Add support for demuxing still HEIC images
On Thu, Oct 5, 2023 at 10:58 AM Andreas Rheinhardt wrote: > > Vignesh Venkat via ffmpeg-devel: > > On Tue, Oct 3, 2023 at 9:40 PM Vittorio Giovara > > wrote: > >> > >> > >> > >> On Wed, Oct 4, 2023 at 12:02 AM Vignesh Venkat via ffmpeg-devel > >> wrote: > >>> > >>> On Tue, Oct 3, 2023 at 6:32 PM Vittorio Giovara > >>> wrote: > >>>> > >>>> On Tue, Oct 3, 2023 at 8:30 PM Steven Liu > >>>> wrote: > >>>> > >>>>>>> 2.42.0.515.g380fc7ccd1-goog > >>>>>>> > >>>>>> > >>>>>> Any comments/objections on merging this? > >>>>> > >>>>> > >>>>> Can this patch support tiled hevc coded or sequence heif？= > >>>>> > >>>> > >>>> I believe that will be possible only after AVStreamGroup is implemented. > >>>> > >>> > >>> Yes, this patch only supports still HEIC images that don't have alpha > >>> and grids (tiles). > >>> > >>> Tiles and Alpha support will be possible only after AVStreamGroup is > >>> implemented. I will look into HEIC sequences in a follow-up. > >>> > >>>> Vignesh is there a sample available? Could we add a test? > >>> > >>> I tested it from the files in > >>> https://github.com/nokiatech/heif/tree/gh-pages/content/images. I am > >>> not sure about HEVC licensing and if we are allowed to copy some of > >>> those files in the ffmpeg fate server. Would generating a random image > >>> with ffmpeg and encoding it as HEIC be good enough? > >> > >> > >> I would prefer a real world example and FATE has a bunch of conformance > >> samples already. > >> Adding the ones from https://github.com/nokiatech/heif_conformance > >> shouldn't be a problem. > >> Ideally the sample test should be added to this same patch. > > > > Great, i have added two samples. Can you please upload C002.heic and > > C003.heic from the heif_conformance repository to the fate server > > under the "heif-conformance" sub-directory. I have also attached those > > two files in this email for reference. I will update the patch with > > the fate tests. > > > > Why are you intend to add so big files when the linked repo contains > smaller files? All five multilayer files are below 20KB each; > multilayer005.heic is even only 4.5KB. MIAF002.heic (8.6KB) and > MIAF003.heic (13.5KB) are also quite small. C025.heic (19.4KB) and > C053.heic (14.2KB) are also quite small and there are also other samples > in the 50KB-60KB range (C017.heic, C018.heic, C019.heic, C020.heic, > C041.heic). Besides taking up less diskspace, smaller files will likely > be faster to decode (which is particularly advantageous for people like > me who run FATE a lot). > There is an excel sheet in the samples github repository that explains the features of each of those files. I picked the files with the simplest features (single item and multi-item) since those are the only two files that I know for certain the current ffmpeg parser can parse as-is. The multi-layer files that you mention may work, but i am not sure if we are handling those layers the right way. So adding something that I am not sure if it has the right behavior or not to a fate test didn't seem right. Also, the fate test only demuxes the files (using -c copy) and does not do any decoding since it tests the parser and not the hevc decoder. So decoding time is not a concern. I do understand the concern about disk-space though. I can probably make a random file that is smaller, but I also thought it was better to use files from an existing conformance repository. Please let me know if that is reasonable. > - Andreas > > ___ > 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". -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH] avcodec/svt-av1: Set force_key_frames only when gop_size == 1
On Wed, Oct 4, 2023 at 10:23 AM Ronald S. Bultje wrote:
>
> Hi,
>
> On Tue, Oct 3, 2023 at 6:53 PM Vignesh Venkatasubramanian via ffmpeg-devel
> wrote:
>>
>> SVT-AV1 does not support requesting keyframes at arbitrary points
>> by setting pic_type to EB_AV1_KEY_PICTURE. So set force_key_frames
>> to 1 only when gop_size == 1.
>>
>> Please see the comments in
>> https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 for a bit more
>> details.
>>
>> Signed-off-by: Vignesh Venkatasubramanian
>> ---
>> libavcodec/libsvtav1.c | 9 +++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
>> index 5015169244..8d2c7f3be4 100644
>> --- a/libavcodec/libsvtav1.c
>> +++ b/libavcodec/libsvtav1.c
>> @@ -253,8 +253,13 @@ static int config_enc_params(EbSvtAv1EncConfiguration
>> *param,
>> // In order for SVT-AV1 to force keyframes by setting pic_type to
>> // EB_AV1_KEY_PICTURE on any frame, force_key_frames has to be set. Note
>> // that this does not force all frames to be keyframes (it only forces a
>> -// keyframe with pic_type is set to EB_AV1_KEY_PICTURE).
>> -param->force_key_frames = 1;
>> +// keyframe with pic_type is set to EB_AV1_KEY_PICTURE). As of now,
>> SVT-AV1
>> +// does not support arbitrary keyframe requests by setting pic_type to
>> +// EB_AV1_KEY_PICTURE, so it is done only when gop_size == 1.
>> +// FIXME: When SVT-AV1 supports arbitrary keyframe requests, this code
>> needs
>> +// to be updated to set force_key_frames accordingly.
>> +if (avctx->gop_size == 1)
>> +param->force_key_frames = 1;
>>
>> if (avctx->framerate.num > 0 && avctx->framerate.den > 0) {
>> param->frame_rate_numerator = avctx->framerate.num;
>> --
>> 2.42.0.582.g8ccd20d70d-goog
>
>
> Seems reasonable to me.
>
> Can you merge, or should I merge for you?
>
Thanks Ronald. I don't have commit access. Can you please merge it?
> Ronald
--
Vignesh
___
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".
Re: [FFmpeg-devel] [PATCH] avformat/mov: Add support for demuxing still HEIC images
On Tue, Oct 3, 2023 at 5:30 PM Steven Liu wrote:
>
> Vignesh Venkat via ffmpeg-devel 于2023年10月4日
> 周三06:57写道：
>
> > On Tue, Sep 26, 2023 at 10:37 AM Vignesh Venkatasubramanian
> > wrote:
> > >
> > > They are similar to AVIF images (both use the HEIF container).
> > > The only additional work needed is to parse the hvcC box and put
> > > it in the extradata.
> > >
> > > With this patch applied, ffmpeg (when built with an HEVC decoder)
> > > is able to decode the files in
> > > https://github.com/nokiatech/heif/tree/gh-pages/content/images
> > >
> > > Partially fixes trac ticket #6521.
> > >
> > > Signed-off-by: Vignesh Venkatasubramanian
> > > ---
> > > libavformat/isom.h | 2 ++
> > > libavformat/mov.c | 38 +-
> > > 2 files changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavformat/isom.h b/libavformat/isom.h
> > > index 3d375d7a46..b30b9da65e 100644
> > > --- a/libavformat/isom.h
> > > +++ b/libavformat/isom.h
> > > @@ -327,6 +327,8 @@ typedef struct MOVContext {
> > > int64_t extent_offset;
> > > } *avif_info;
> > > int avif_info_size;
> > > +int64_t hvcC_offset;
> > > +int hvcC_size;
> > > int interleaved_read;
> > > } MOVContext;
> > >
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index 1996e0028c..cec9cb5fe1 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -1218,7 +1218,8 @@ static int mov_read_ftyp(MOVContext *c,
> > AVIOContext *pb, MOVAtom atom)
> > > c->isom = 1;
> > > av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand:
> > %.4s\n",(char *));
> > > av_dict_set(>fc->metadata, "major_brand", type, 0);
> > > -c->is_still_picture_avif = !strncmp(type, "avif", 4);
> > > +c->is_still_picture_avif = !strncmp(type, "avif", 4) ||
> > > + !strncmp(type, "mif1", 4);
> > > minor_ver = avio_rb32(pb); /* minor version */
> > > av_dict_set_int(>fc->metadata, "minor_version", minor_ver, 0);
> > >
> > > @@ -4911,6 +4912,16 @@ static int avif_add_stream(MOVContext *c, int
> > item_id)
> > > st->priv_data = sc;
> > > st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> > > st->codecpar->codec_id = AV_CODEC_ID_AV1;
> > > +if (c->hvcC_offset >= 0) {
> > > +int ret;
> > > +int64_t pos = avio_tell(c->fc->pb);
> > > +st->codecpar->codec_id = AV_CODEC_ID_HEVC;
> > > +avio_seek(c->fc->pb, c->hvcC_offset, SEEK_SET);
> > > +ret = ff_get_extradata(c->fc, st->codecpar, c->fc->pb,
> > c->hvcC_size);
> > > +if (ret < 0)
> > > +return ret;
> > > +avio_seek(c->fc->pb, pos, SEEK_SET);
> > > +}
> > > sc->ffindex = st->index;
> > > c->trak_index = st->index;
> > > st->avg_frame_rate.num = st->avg_frame_rate.den = 1;
> > > @@ -4953,6 +4964,8 @@ static int avif_add_stream(MOVContext *c, int
> > item_id)
> > >
> > > static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > > {
> > > +c->hvcC_offset = -1;
> > > +c->hvcC_size = 0;
> > > while (atom.size > 8) {
> > > uint32_t tag;
> > > if (avio_feof(pb))
> > > @@ -7826,6 +7839,28 @@ static int mov_read_iloc(MOVContext *c,
> > AVIOContext *pb, MOVAtom atom)
> > > return atom.size;
> > > }
> > >
> > > +static int mov_read_iprp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > > +{
> > > +int size = avio_rb32(pb);
> > > +if (avio_rl32(pb) != MKTAG('i','p','c','o'))
> > > +return AVERROR_INVALIDDATA;
> > > +size -= 8;
> > > +while (size > 0) {
> > > +int sub_size, sub_type;
> > > +sub_size = avio_rb32(pb);
> > > +sub_type = avio_rl32(pb);
> > > +sub_size -= 8;
> > > +size -= sub_size + 8;
> > > +if (sub_type == MKTAG('h','v','c','C')) {
> > > +c->hvcC_offset = avio_tell(pb);
> > > +c->hvcC_size = sub_size
Re: [FFmpeg-devel] [PATCH 3/5] avformat/mov: Better check for duplicate iloc
On Tue, Oct 3, 2023 at 3:56 PM Vignesh Venkat wrote:
>
> On Fri, Sep 29, 2023 at 12:21 PM Michael Niedermayer
> wrote:
> >
> > On Tue, Apr 25, 2023 at 03:22:50PM -0700, Vignesh Venkatasubramanian wrote:
> > > On Mon, Apr 17, 2023 at 4:18 PM Michael Niedermayer
> > > wrote:
> > > >
> > > > On Mon, Apr 17, 2023 at 12:36:26PM +0200, Anton Khirnov wrote:
> > > > > Quoting Michael Niedermayer (2023-04-17 00:25:16)
> > > > > > Fixes: memleak
> > > > > > Fixes:
> > > > > > 45982/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6674082962997248
> > > > > >
> > > > > > Found-by: continuous fuzzing process
> > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer
> > > > > > ---
> > > > > > libavformat/mov.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > > > > index 057fd872b10..6853bb324cf 100644
> > > > > > --- a/libavformat/mov.c
> > > > > > +++ b/libavformat/mov.c
> > > > > > @@ -,7 +,7 @@ static int mov_read_iloc(MOVContext *c,
> > > > > > AVIOContext *pb, MOVAtom atom)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > -if (c->fc->nb_streams) {
> > > > > > +if (c->fc->nb_streams || c->avif_info) {
> > > > >
> > > > > This first condition is now redundant, is it not?
> > > >
> > > > Iam not sure
> > >
> > > I think the second condition alone should be enough here. Either way,
> > > lgtm (if the current patch is more clearer for readers).
> >
> > Ill apply with teh first condition converted to an assert then
> >
> >
>
> sounds good.
>
> > >
> > > > what exactly happens if a trak occurs before
> > > >
> > >
> > > If a trak occurs before, then the condition in the line above should
> > > take care of that case (!c->is_still_picture_avif). Because if a trak
> > > was found, it will not be considered a still picture.
> > >
> > > > Iam also not sure what happens if multiple meta tags occur triggering
> > > > the avif stream addition, i may be missing something but the code seems
> > > > not to expect that
> > > >
> > >
> > > Multiple meta tags are not allowed in the AVIF/HEIF specification.
> >
> > sure but what happens if the occur anyway, does the code handle that
> > with no undefined behavior ?
> >
>
> yeah, the current code will treat each meta tag as a separate track
> (AVStream). This should be disallowed, i will send a patch to error
> out if more than one top-level meta box is seen.
>
The patch for disallowing multiple meta boxes is here:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20231003230423.951161-1-vigne...@google.com/
> > thx
> >
> > [...]
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > I have often repented speaking, but never of holding my tongue.
> > -- Xenocrates
> > ___
> > 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".
>
>
>
> --
> Vignesh
--
Vignesh
___
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".
Re: [FFmpeg-devel] [PATCH] avformat/mov: Add support for demuxing still HEIC images
On Tue, Oct 3, 2023 at 7:35 PM Leo Izen wrote:
>
> On 9/26/23 13:37, Vignesh Venkatasubramanian via ffmpeg-devel wrote:
> > They are similar to AVIF images (both use the HEIF container).
> > The only additional work needed is to parse the hvcC box and put
> > it in the extradata.
> >
> > With this patch applied, ffmpeg (when built with an HEVC decoder)
> > is able to decode the files in
> > https://github.com/nokiatech/heif/tree/gh-pages/content/images
> >
> > Partially fixes trac ticket #6521.
> >
> > Signed-off-by: Vignesh Venkatasubramanian
> > ---
> > libavformat/isom.h | 2 ++
> > libavformat/mov.c | 38 +-
> > 2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/isom.h b/libavformat/isom.h
> > index 3d375d7a46..b30b9da65e 100644
> > --- a/libavformat/isom.h
> > +++ b/libavformat/isom.h
> > @@ -327,6 +327,8 @@ typedef struct MOVContext {
> > int64_t extent_offset;
> > } *avif_info;
> > int avif_info_size;
> > +int64_t hvcC_offset;
> > +int hvcC_size;
> > int interleaved_read;
> > } MOVContext;
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 1996e0028c..cec9cb5fe1 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -1218,7 +1218,8 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext
> > *pb, MOVAtom atom)
> > c->isom = 1;
> > av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand:
> > %.4s\n",(char *));
> > av_dict_set(>fc->metadata, "major_brand", type, 0);
> > -c->is_still_picture_avif = !strncmp(type, "avif", 4);
> > +c->is_still_picture_avif = !strncmp(type, "avif", 4) ||
> > + !strncmp(type, "mif1", 4);
>
> This appears to be an unrelated change. Is it?
>
No, "mif1" is the major brand reported by HEIC files. So this is
needed to detect HEIC files.
> > minor_ver = avio_rb32(pb); /* minor version */
> > av_dict_set_int(>fc->metadata, "minor_version", minor_ver, 0);
> >
> > @@ -4911,6 +4912,16 @@ static int avif_add_stream(MOVContext *c, int
> > item_id)
> > st->priv_data = sc;
> > st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> > st->codecpar->codec_id = AV_CODEC_ID_AV1;
> > +if (c->hvcC_offset >= 0) {
> > +int ret;
> > +int64_t pos = avio_tell(c->fc->pb);
> > +st->codecpar->codec_id = AV_CODEC_ID_HEVC;
> > +avio_seek(c->fc->pb, c->hvcC_offset, SEEK_SET);
> > +ret = ff_get_extradata(c->fc, st->codecpar, c->fc->pb,
> > c->hvcC_size);
> > +if (ret < 0)
> > +return ret;
> > +avio_seek(c->fc->pb, pos, SEEK_SET);
> > +}
>
> Will this fail on non-seekable input? If it does, do we care?
>
I tried this with a pipe as an input (cat file.heic | ffmpeg -i - ...)
and it worked. I also see plenty of unconditional avio_seek calls in
this file. So I am not sure how it works.
Anyways, I added a check to fail here if the seek does not take us to
the desired position.
> > sc->ffindex = st->index;
> > c->trak_index = st->index;
> > st->avg_frame_rate.num = st->avg_frame_rate.den = 1;
> > @@ -4953,6 +4964,8 @@ static int avif_add_stream(MOVContext *c, int item_id)
> >
> > static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > {
> > +c->hvcC_offset = -1;
> > +c->hvcC_size = 0;
> > while (atom.size > 8) {
> > uint32_t tag;
> > if (avio_feof(pb))
> > @@ -7826,6 +7839,28 @@ static int mov_read_iloc(MOVContext *c, AVIOContext
> > *pb, MOVAtom atom)
> > return atom.size;
> > }
> >
> > +static int mov_read_iprp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > +{
> > +int size = avio_rb32(pb);
> > +if (avio_rl32(pb) != MKTAG('i','p','c','o'))
> > +return AVERROR_INVALIDDATA;
>
> Is there a reason you use ipco here instead of iprp? I'm not saying this
> is wrong, but I am confused while you're doing it this way.
>
The control flow will jump into this function only when an "iprp" box
has been seen (pb points to the start of the contents of the iprp
box). "ipco" is a sub-box of the "iprp" box that we care about. We
cannot register sub-boxes to mov_read_default without registering the
containing box, as the containing box will be skipped if it is
unknown. Some other sub-box reading is already implemented this way
(mov_read_meta for example).
> > +size -= 8;
> > +while (size > 0) {
> > +int sub_size, sub_type;
> > +sub_size = avio_rb32(pb);
> > +sub_type = avio_rl32(pb);
> > +sub_size -= 8;
> > +size -= sub_size + 8;
> > +if (sub_type == MKTAG('h','v','c','C')) {
> > +c->hvcC_offset = avio_tell(pb);
> > +c->hvcC_size = sub_size;
> > +break;
> > +}
>
> Are these permitted to use extended-size tags? i.e. size = 1, followed
> by a big-endian 64-bit size, then followed by the tag?
>
MP4 "type" fields are always
Re: [FFmpeg-devel] [PATCH] avformat/mov: Add support for demuxing still HEIC images
On Tue, Oct 3, 2023 at 6:32 PM Vittorio Giovara wrote: > > On Tue, Oct 3, 2023 at 8:30 PM Steven Liu wrote: > > > > > 2.42.0.515.g380fc7ccd1-goog > > > > > > > > > > Any comments/objections on merging this? > > > > > > Can this patch support tiled hevc coded or sequence heif？= > > > > I believe that will be possible only after AVStreamGroup is implemented. > Yes, this patch only supports still HEIC images that don't have alpha and grids (tiles). Tiles and Alpha support will be possible only after AVStreamGroup is implemented. I will look into HEIC sequences in a follow-up. > Vignesh is there a sample available? Could we add a test? I tested it from the files in https://github.com/nokiatech/heif/tree/gh-pages/content/images. I am not sure about HEVC licensing and if we are allowed to copy some of those files in the ffmpeg fate server. Would generating a random image with ffmpeg and encoding it as HEIC be good enough? > -- > Vittorio > ___ > 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". -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH] avformat/mov: Add support for demuxing still HEIC images
On Tue, Sep 26, 2023 at 10:37 AM Vignesh Venkatasubramanian
wrote:
>
> They are similar to AVIF images (both use the HEIF container).
> The only additional work needed is to parse the hvcC box and put
> it in the extradata.
>
> With this patch applied, ffmpeg (when built with an HEVC decoder)
> is able to decode the files in
> https://github.com/nokiatech/heif/tree/gh-pages/content/images
>
> Partially fixes trac ticket #6521.
>
> Signed-off-by: Vignesh Venkatasubramanian
> ---
> libavformat/isom.h | 2 ++
> libavformat/mov.c | 38 +-
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index 3d375d7a46..b30b9da65e 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -327,6 +327,8 @@ typedef struct MOVContext {
> int64_t extent_offset;
> } *avif_info;
> int avif_info_size;
> +int64_t hvcC_offset;
> +int hvcC_size;
> int interleaved_read;
> } MOVContext;
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 1996e0028c..cec9cb5fe1 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1218,7 +1218,8 @@ static int mov_read_ftyp(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> c->isom = 1;
> av_log(c->fc, AV_LOG_DEBUG, "ISO: File Type Major Brand: %.4s\n",(char
> *));
> av_dict_set(>fc->metadata, "major_brand", type, 0);
> -c->is_still_picture_avif = !strncmp(type, "avif", 4);
> +c->is_still_picture_avif = !strncmp(type, "avif", 4) ||
> + !strncmp(type, "mif1", 4);
> minor_ver = avio_rb32(pb); /* minor version */
> av_dict_set_int(>fc->metadata, "minor_version", minor_ver, 0);
>
> @@ -4911,6 +4912,16 @@ static int avif_add_stream(MOVContext *c, int item_id)
> st->priv_data = sc;
> st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> st->codecpar->codec_id = AV_CODEC_ID_AV1;
> +if (c->hvcC_offset >= 0) {
> +int ret;
> +int64_t pos = avio_tell(c->fc->pb);
> +st->codecpar->codec_id = AV_CODEC_ID_HEVC;
> +avio_seek(c->fc->pb, c->hvcC_offset, SEEK_SET);
> +ret = ff_get_extradata(c->fc, st->codecpar, c->fc->pb, c->hvcC_size);
> +if (ret < 0)
> +return ret;
> +avio_seek(c->fc->pb, pos, SEEK_SET);
> +}
> sc->ffindex = st->index;
> c->trak_index = st->index;
> st->avg_frame_rate.num = st->avg_frame_rate.den = 1;
> @@ -4953,6 +4964,8 @@ static int avif_add_stream(MOVContext *c, int item_id)
>
> static int mov_read_meta(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> {
> +c->hvcC_offset = -1;
> +c->hvcC_size = 0;
> while (atom.size > 8) {
> uint32_t tag;
> if (avio_feof(pb))
> @@ -7826,6 +7839,28 @@ static int mov_read_iloc(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> return atom.size;
> }
>
> +static int mov_read_iprp(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +int size = avio_rb32(pb);
> +if (avio_rl32(pb) != MKTAG('i','p','c','o'))
> +return AVERROR_INVALIDDATA;
> +size -= 8;
> +while (size > 0) {
> +int sub_size, sub_type;
> +sub_size = avio_rb32(pb);
> +sub_type = avio_rl32(pb);
> +sub_size -= 8;
> +size -= sub_size + 8;
> +if (sub_type == MKTAG('h','v','c','C')) {
> +c->hvcC_offset = avio_tell(pb);
> +c->hvcC_size = sub_size;
> +break;
> +}
> +avio_skip(pb, sub_size);
> +}
> +return atom.size;
> +}
> +
> static const MOVParseTableEntry mov_default_parse_table[] = {
> { MKTAG('A','C','L','R'), mov_read_aclr },
> { MKTAG('A','P','R','G'), mov_read_avid },
> @@ -7933,6 +7968,7 @@ static const MOVParseTableEntry
> mov_default_parse_table[] = {
> { MKTAG('p','c','m','C'), mov_read_pcmc }, /* PCM configuration box */
> { MKTAG('p','i','t','m'), mov_read_pitm },
> { MKTAG('e','v','c','C'), mov_read_glbl },
> +{ MKTAG('i','p','r','p'), mov_read_iprp },
> { 0, NULL }
> };
>
> --
> 2.42.0.515.g380fc7ccd1-goog
>
Any comments/objections on merging this?
--
Vignesh
___
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".
Re: [FFmpeg-devel] [PATCH 3/5] avformat/mov: Better check for duplicate iloc
On Fri, Sep 29, 2023 at 12:21 PM Michael Niedermayer
wrote:
>
> On Tue, Apr 25, 2023 at 03:22:50PM -0700, Vignesh Venkatasubramanian wrote:
> > On Mon, Apr 17, 2023 at 4:18 PM Michael Niedermayer
> > wrote:
> > >
> > > On Mon, Apr 17, 2023 at 12:36:26PM +0200, Anton Khirnov wrote:
> > > > Quoting Michael Niedermayer (2023-04-17 00:25:16)
> > > > > Fixes: memleak
> > > > > Fixes:
> > > > > 45982/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6674082962997248
> > > > >
> > > > > Found-by: continuous fuzzing process
> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer
> > > > > ---
> > > > > libavformat/mov.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > > > index 057fd872b10..6853bb324cf 100644
> > > > > --- a/libavformat/mov.c
> > > > > +++ b/libavformat/mov.c
> > > > > @@ -,7 +,7 @@ static int mov_read_iloc(MOVContext *c,
> > > > > AVIOContext *pb, MOVAtom atom)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -if (c->fc->nb_streams) {
> > > > > +if (c->fc->nb_streams || c->avif_info) {
> > > >
> > > > This first condition is now redundant, is it not?
> > >
> > > Iam not sure
> >
> > I think the second condition alone should be enough here. Either way,
> > lgtm (if the current patch is more clearer for readers).
>
> Ill apply with teh first condition converted to an assert then
>
>
sounds good.
> >
> > > what exactly happens if a trak occurs before
> > >
> >
> > If a trak occurs before, then the condition in the line above should
> > take care of that case (!c->is_still_picture_avif). Because if a trak
> > was found, it will not be considered a still picture.
> >
> > > Iam also not sure what happens if multiple meta tags occur triggering
> > > the avif stream addition, i may be missing something but the code seems
> > > not to expect that
> > >
> >
> > Multiple meta tags are not allowed in the AVIF/HEIF specification.
>
> sure but what happens if the occur anyway, does the code handle that
> with no undefined behavior ?
>
yeah, the current code will treat each meta tag as a separate track
(AVStream). This should be disallowed, i will send a patch to error
out if more than one top-level meta box is seen.
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I have often repented speaking, but never of holding my tongue.
> -- Xenocrates
> ___
> 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".
--
Vignesh
___
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".
Re: [FFmpeg-devel] [PATCH] avcodec/svt-av1: Set pic_type only when gop_size == 1
On Thu, Sep 28, 2023 at 2:05 AM Ronald S. Bultje wrote: > > Hi Vignesh, > > On Thu, Sep 28, 2023 at 12:14 AM Vignesh Venkatasubramanian via ffmpeg-devel > wrote: >> >> SVT-AV1 does not support requesting keyframes at arbitrary points >> by setting pic_type to EB_AV1_KEY_PICTURE. >> >> This patch changes the following: >> * Set pic_type to EB_AV1_KEY_PICTURE only when gop_size == 1. This >>only has an effect in this case (combined with force_key_frames). >>In all other cases, setting this has no effect. >> * Set force_key_frames to 1 only when gop_size == 1, this is >>needed for pic_type request above to work. >> >> Please see the comments in >> https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 for a bit more >> details. > > > Right. So, if I put my archeologist hat on, is it fair to say that what > probably happened is that force_key_frames used to not exist, and pic_type > worked as per above code. Then force_key_frames was required (because of > quality implications), breaking the above code. And now we're removing the > broken code because otherwise there's a quality penalty. Is that fair? > Hi Ronald, Yes, i believe your assessment is correct. > I agree it's probably unfair to ask you to fix the broken use case that was > previously possible (I suppose this comes down to exposing an option to set > force_key_frames by having the user indicate s/he'll be using pic_type). > However, maybe we should not remove the dead code, because it's functionally > OK. Maybe we even need a FIXME above the line where we set force_key_frames > only if gop_size==1, explaining this is a bug. WDYT? > > (It feels a bit weird to remove it because it's broken, is what I'm trying to > say, especially because we know how to fix it.) > Makes sense, i have kept the old code in the pic_type setting part and have updated this patch to only set force_key_frames when gop_size == 1. PTAL. > Ronald -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH] libsvtav1: Add workaround for gop_size == 1
On Wed, Jun 28, 2023 at 4:27 PM James Almer wrote: > > On 6/28/2023 8:25 PM, Ronald S. Bultje wrote: > > Hi, > > > > On Wed, Jun 28, 2023 at 12:48 PM Vignesh Venkat < > > vigneshv-at-google@ffmpeg.org> wrote: > > > >> i will update the upstream bug to clarify this part. but in the > >> meantime, i think this patch is reasonable to replicate the behavior > >> of other AV1 encoders with -g 1. > >> > > > > Sounds reasonable, I think. James, objections? > > > > Ronald > > No, fine by me. > ___ > 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". Ronald or James, would you be able to merge this please? :) -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH] libsvtav1: Add workaround for gop_size == 1
On Tue, Jun 27, 2023 at 12:07 PM Ronald S. Bultje wrote: > > Hi Vignesh, > > On Tue, Jun 27, 2023 at 1:55 PM Vignesh Venkat < > vigneshv-at-google@ffmpeg.org> wrote: > > > > >>> In some versions of libsvtav1, setting intra_period_length to 0 > > > [..] > > > > >>> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 > > > >>> > > > >>> Example command: ffmpeg -f lavfi -i > > testsrc=duration=1:size=64x64:rate=30 -c:v libsvtav1 -g 1 -y test.webm > > > >>> > > > >>> Before: Only first frame is keyframe. > > > >>> After: All frames are keyframes. > > > [..] > > > From what i tested, it seems to be there on the tip of tree SVT-AV1 as > > well. So we can't guard it with macros until the issue is fixed. > > > > Isn't this the expected behaviour, btw? > > See https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/master/Docs/Parameters.md > > "Keyint --keyint [-2-(2^31)-1] -2 GOP size (frames), use s suffix for > seconds (SvtAv1EncApp only) [-2: ~5 seconds, -1: "infinite" only for CRF, > 0: == -1] " > > note the final parts: 0=-1, and -1 means "infinite" keyint (i.e. never > insert a new KF). Since FFmpeg's wrapper sets keyint to "gopsize value > minus 1", "-g 1" seems to become equivalent to "--keyint 0" on the SVT-AV1 > commandline, unless there's some discrepancy between API and CLI interface. > Yes, it might be working as documented in the SVT-AV1 API. But the point is that we would need a way to force all key frames (as i think that is what a user would mean when they set "-g 1" in ffmpeg?). > (I admit that if I set it to 1, I don't actually get keyframes, but rather > intraonly frames for all except the first frame. --keyint 2 correctly uses > key - inter pairs. But your "before" statement above appeared to suggest > that the first keyframe is followed by inter frames, not intraonly frames. > If you meant "intraonly after key" instead of "inter after key", you should > probably note that more explicitly. This may also help upstream reproduce > in #2076 so they can fix it going forward.) yeah i wasn't really paying attention to whether the subsequent frames were intra-only or inter frames. i was just looking at whether or not they were keyframes. :) i will update the upstream bug to clarify this part. but in the meantime, i think this patch is reasonable to replicate the behavior of other AV1 encoders with -g 1. > > Ronald > ___ > 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". -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH] libsvtav1: Add workaround for gop_size == 1
On Mon, Jun 26, 2023 at 4:53 PM James Almer wrote: > > On 6/26/2023 7:32 PM, Vignesh Venkat wrote: > > On Mon, Jun 26, 2023 at 3:17 PM Ronald S. Bultje wrote: > >> > >> Hi, > >> > >> On Mon, Jun 26, 2023 at 1:47 PM Vignesh Venkatasubramanian > >> wrote: > >>> > >>> In some versions of libsvtav1, setting intra_period_length to 0 > >>> does not produce the intended result (i.e.) all frames produced > >>> are not keyframes. > >>> > >>> Instead handle the gop_size == 1 as a special case by setting > >>> the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so > >>> that all the output frames are keyframes. > >>> > >>> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 > >>> > >>> Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 > >>> -c:v libsvtav1 -g 1 -y test.webm > >>> > >>> Before: Only first frame is keyframe. > >>> After: All frames are keyframes. > >>> > >>> Signed-off-by: Vignesh Venkatasubramanian > >>> --- > >>> libavcodec/libsvtav1.c | 16 +++- > >>> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >> > >> It feels a bit dirty to add workarounds in ffmpeg library wrappers for > >> bugs in these libraries. Not 100% against it, but it's not particularly > >> great. > >> > > > > Yeah I agree that it's not ideal. But minor changes like this that are > > commented/documented is okay i believe. > > What are the buggy svt-av1 versions? Can this be wrapped in a > preprocessor check, so that if we bump the minimum required version in > the future and it's newer, this change can be removed/undone? > From what i tested, it seems to be there on the tip of tree SVT-AV1 as well. So we can't guard it with macros until the issue is fixed. ___ > 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". -- Vignesh ___ 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".
Re: [FFmpeg-devel] [PATCH] libsvtav1: Add workaround for gop_size == 1
On Mon, Jun 26, 2023 at 3:17 PM Ronald S. Bultje wrote: > > Hi, > > On Mon, Jun 26, 2023 at 1:47 PM Vignesh Venkatasubramanian > wrote: >> >> In some versions of libsvtav1, setting intra_period_length to 0 >> does not produce the intended result (i.e.) all frames produced >> are not keyframes. >> >> Instead handle the gop_size == 1 as a special case by setting >> the pic_type to EB_AV1_KEY_PICTURE when encoding each frame so >> that all the output frames are keyframes. >> >> SVT-AV1 Bug: https://gitlab.com/AOMediaCodec/SVT-AV1/-/issues/2076 >> >> Example command: ffmpeg -f lavfi -i testsrc=duration=1:size=64x64:rate=30 >> -c:v libsvtav1 -g 1 -y test.webm >> >> Before: Only first frame is keyframe. >> After: All frames are keyframes. >> >> Signed-off-by: Vignesh Venkatasubramanian >> --- >> libavcodec/libsvtav1.c | 16 +++- >> 1 file changed, 15 insertions(+), 1 deletion(-) > > > It feels a bit dirty to add workarounds in ffmpeg library wrappers for bugs > in these libraries. Not 100% against it, but it's not particularly great. > Yeah I agree that it's not ideal. But minor changes like this that are commented/documented is okay i believe. > Ronald -- Vignesh ___ 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".
