[FFmpeg-devel] [TC] "Future Log Output Default"

[FFmpeg-devel] [TC] "Future Log Output Default"

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1409 bytes --]

Hi all

This is just an announcement that the TC has been asked to look into
    avutil/log: Add log flag to control printing of memory addresses
    GitHub:    https://github.com/ffstaging/FFmpeg/pull/59
    Patchwork: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14094
    ...

and the disagreement between people about it.

So far, Niklas, Martin and myself have commented, there have been no formal
decissions and no votes, we just since yesterday send some comments.

From these to me it seems the TC members who spoke so far seem to agree
that the addresses in the log are "mostly noise".

I wonder a bit if the TC discussions should/could be more public but thats
a subject for a different thread. Myself summarizing will surely miss some
fine points but iam trying anyway.

Niklas also brought up that item_name() is used in filters to disambiguate
instances and that we maybe could add a const char *name and have fftools
genrate a unique ID for each

Either way none of this is a TC decission or binding its just a partial
summary of the things that TC members said

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [TC] "Future Log Output Default"

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1796 bytes --]

On Sat, Apr 26, 2025 at 05:10:27PM +0200, Michael Niedermayer wrote:
> Hi all
> 
> This is just an announcement that the TC has been asked to look into
>     avutil/log: Add log flag to control printing of memory addresses
>     GitHub:    https://github.com/ffstaging/FFmpeg/pull/59
>     Patchwork: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14094
>     ...
> 
> and the disagreement between people about it.
> 
> So far, Niklas, Martin and myself have commented, there have been no formal
> decissions and no votes, we just since yesterday send some comments.
> 
> From these to me it seems the TC members who spoke so far seem to agree
> that the addresses in the log are "mostly noise".
> 
> I wonder a bit if the TC discussions should/could be more public but thats
> a subject for a different thread. Myself summarizing will surely miss some
> fine points but iam trying anyway.
> 
> Niklas also brought up that item_name() is used in filters to disambiguate
> instances and that we maybe could add a const char *name and have fftools
> genrate a unique ID for each
> 
> Either way none of this is a TC decission or binding its just a partial
> summary of the things that TC members said

also forgot, martin said hes quite short on time and asked me if i could
take over this case.
and i had a bit of time today after yesterday having to collect indoor
rain in my bathroom from faulty plumbing. So i posted above summary
of the current state and as TC should announce when it starts working
on a case
if some other TC member wants to take this over, iam happy to hand it over

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [TC] "Future Log Output Default"

[softworkz .]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]



> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Samstag, 26. April 2025 17:10
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: [FFmpeg-devel] [TC] "Future Log Output Default"
> 
> Hi all
> 
> This is just an announcement that the TC has been asked to look into
>     avutil/log: Add log flag to control printing of memory addresses
>     GitHub:    https://github.com/ffstaging/FFmpeg/pull/59
>     Patchwork:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14094
>     ...
> 
> and the disagreement between people about it.
> 
> So far, Niklas, Martin and myself have commented, there have been no
> formal
> decissions and no votes, we just since yesterday send some comments.
> 
> From these to me it seems the TC members who spoke so far seem to
> agree
> that the addresses in the log are "mostly noise".
> 
> I wonder a bit if the TC discussions should/could be more public but
> thats
> a subject for a different thread. Myself summarizing will surely miss
> some
> fine points but iam trying anyway.
> 
> Niklas also brought up that item_name() is used in filters to
> disambiguate
> instances and that we maybe could add a const char *name and have
> fftools
> genrate a unique ID for each
> 
> Either way none of this is a TC decission or binding its just a
> partial
> summary of the things that TC members said
> 
> thx
> 
> --
> Michael     GnuPG fingerprint:



Thanks Michael,
hello everybody,

I would like to note that my reason for handing this over to the TC was not primarily about conflict resolution, it's rather that I feel that this decision is at least one size too big for me standing right in the front line.

Ideally, I think, it would be great when it would be backed by a majority, but I wouldn't know how to determine that other than a vote. The TC in turn has already been elected for making those decisions and so that seems to be the most natural way. I'm fine with any outcome, also open to do once another variant of implementation if that would be desired, or just keep the default to current behavior if that's the verdict.

Thanks
sw



[-- Attachment #2: Type: message/rfc822, Size: 19229 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 8675 bytes --]

On Tue, Apr 22, 2025 at 04:44:07PM -0500, Romain Beauxis wrote:
> ---
>  tests/Makefile                             |   4 +
>  tests/api/Makefile                         |   2 +-
>  tests/api/api-dump-stream-meta-test.c      | 177 +++++++++++++++++++++
>  tests/fate/ogg-flac.mak                    |  11 ++
>  tests/fate/ogg-opus.mak                    |  11 ++
>  tests/fate/ogg-vorbis.mak                  |  11 ++
>  tests/ref/fate/ogg-flac-chained-meta.txt   |  12 ++
>  tests/ref/fate/ogg-opus-chained-meta.txt   |  27 ++++
>  tests/ref/fate/ogg-vorbis-chained-meta.txt |  17 ++
>  9 files changed, 271 insertions(+), 1 deletion(-)
>  create mode 100644 tests/api/api-dump-stream-meta-test.c
>  create mode 100644 tests/fate/ogg-flac.mak
>  create mode 100644 tests/fate/ogg-opus.mak
>  create mode 100644 tests/fate/ogg-vorbis.mak
>  create mode 100644 tests/ref/fate/ogg-flac-chained-meta.txt
>  create mode 100644 tests/ref/fate/ogg-opus-chained-meta.txt
>  create mode 100644 tests/ref/fate/ogg-vorbis-chained-meta.txt
>
> diff --git a/tests/Makefile b/tests/Makefile
> index f9f5fc07f3..75b9bcc729 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -219,6 +219,9 @@ include $(SRC_PATH)/tests/fate/mpeg4.mak
>  include $(SRC_PATH)/tests/fate/mpegps.mak
>  include $(SRC_PATH)/tests/fate/mpegts.mak
>  include $(SRC_PATH)/tests/fate/mxf.mak
> +include $(SRC_PATH)/tests/fate/ogg-vorbis.mak
> +include $(SRC_PATH)/tests/fate/ogg-flac.mak
> +include $(SRC_PATH)/tests/fate/ogg-opus.mak
>  include $(SRC_PATH)/tests/fate/oma.mak
>  include $(SRC_PATH)/tests/fate/opus.mak
>  include $(SRC_PATH)/tests/fate/pcm.mak
> @@ -277,6 +280,7 @@ $(FATE_FFPROBE) $(FATE_FFMPEG_FFPROBE) $(FATE_SAMPLES_FFPROBE) $(FATE_SAMPLES_FF
>  $(FATE_SAMPLES_FASTSTART): tools/qt-faststart$(EXESUF)
>  $(FATE_SAMPLES_DUMP_DATA) $(FATE_SAMPLES_DUMP_DATA-yes): tools/venc_data_dump$(EXESUF)
>  $(FATE_SAMPLES_SCALE_SLICE): tools/scale_slice_test$(EXESUF)
> +$(FATE_SAMPLES_DUMP_STREAM_META): tests/api/api-dump-stream-meta-test$(EXESUF)
>
>  ifdef SAMPLES
>  FATE += $(FATE_EXTERN)
> diff --git a/tests/api/Makefile b/tests/api/Makefile
> index c96e636756..a2cb06a729 100644
> --- a/tests/api/Makefile
> +++ b/tests/api/Makefile
> @@ -1,7 +1,7 @@
>  APITESTPROGS-$(call ENCDEC, FLAC, FLAC) += api-flac
>  APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264
>  APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264-slice
> -APITESTPROGS-yes += api-seek
> +APITESTPROGS-yes += api-seek api-dump-stream-meta
>  APITESTPROGS-$(call DEMDEC, H263, H263) += api-band
>  APITESTPROGS-$(HAVE_THREADS) += api-threadmessage
>  APITESTPROGS += $(APITESTPROGS-yes)
> diff --git a/tests/api/api-dump-stream-meta-test.c b/tests/api/api-dump-stream-meta-test.c
> new file mode 100644
> index 0000000000..bbfbd1f30b
> --- /dev/null
> +++ b/tests/api/api-dump-stream-meta-test.c
> @@ -0,0 +1,177 @@
> +/*
> + * Copyright (c) 2025 Romain Beauxis
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +/**
> + * Dump stream metadata
> + */
> +
> +#include "libavcodec/avcodec.h"
> +#include "libavformat/avformat.h"
> +#include "libavutil/timestamp.h"
> +
> +static int dump_stream_meta(const char *input_filename) {
> +    const AVCodec *codec = NULL;
> +    AVPacket *pkt = NULL;
> +    AVFrame *fr = NULL;
> +    AVFormatContext *fmt_ctx = NULL;
> +    AVCodecContext *ctx = NULL;
> +    AVCodecParameters *origin_par = NULL;
> +    AVStream *st;
> +    int stream_idx = 0;
> +    int result;
> +    char *metadata;
> +
> +    result = avformat_open_input(&fmt_ctx, input_filename, NULL, NULL);
> +    if (result < 0) {
> +        av_log(NULL, AV_LOG_ERROR, "Can't open file\n");
> +        return result;
> +    }
> +
> +    result = avformat_find_stream_info(fmt_ctx, NULL);
> +    if (result < 0) {
> +        av_log(NULL, AV_LOG_ERROR, "Can't get stream info\n");
> +        goto end;
> +    }
> +
> +    if (fmt_ctx->nb_streams > 1) {
> +        av_log(NULL, AV_LOG_ERROR, "More than one stream found in input!\n");
> +        goto end;
> +    }
> +
> +    origin_par = fmt_ctx->streams[stream_idx]->codecpar;
> +    st = fmt_ctx->streams[stream_idx];
> +
> +    result = av_dict_get_string(st->metadata, &metadata, '=', ':');
> +    if (result < 0)
> +        goto end;
> +
> +    printf("Stream ID: %d, codec name: %s, metadata: %s\n", stream_idx,
> +           avcodec_get_name(origin_par->codec_id),
> +           strlen(metadata) ? metadata : "N/A");
> +
> +    codec = avcodec_find_decoder(origin_par->codec_id);
> +    if (!codec) {
> +        av_log(NULL, AV_LOG_ERROR, "Can't find decoder\n");
> +        result = AVERROR_DECODER_NOT_FOUND;
> +        goto end;
> +    }
> +
> +    ctx = avcodec_alloc_context3(codec);
> +    if (!ctx) {
> +        av_log(NULL, AV_LOG_ERROR, "Can't allocate decoder context\n");
> +        result = AVERROR(ENOMEM);
> +        goto end;
> +    }
> +
> +    result = avcodec_parameters_to_context(ctx, origin_par);
> +    if (result) {
> +        av_log(NULL, AV_LOG_ERROR, "Can't copy decoder context\n");
> +        goto end;
> +    }
> +
> +    result = avcodec_open2(ctx, codec, NULL);
> +    if (result < 0) {
> +        av_log(ctx, AV_LOG_ERROR, "Can't open decoder\n");
> +        goto end;
> +    }
> +
> +    pkt = av_packet_alloc();
> +    if (!pkt) {
> +        av_log(NULL, AV_LOG_ERROR, "Cannot allocate packet\n");
> +        result = AVERROR(ENOMEM);
> +        goto end;
> +    }
> +
> +    fr = av_frame_alloc();
> +    if (!fr) {
> +        av_log(NULL, AV_LOG_ERROR, "Can't allocate frame\n");
> +        result = AVERROR(ENOMEM);
> +        goto end;
> +    }
> +
> +    for (;;) {
> +        result = av_read_frame(fmt_ctx, pkt);
> +        if (result)
> +            goto end;
> +
> +        if (pkt->stream_index != stream_idx) {
> +            av_packet_unref(pkt);
> +            continue;
> +        }
> +
> +        printf("Stream ID: %d, packet PTS: %s, packet DTS: %s\n",
> +               pkt->stream_index, av_ts2str(pkt->pts), av_ts2str(pkt->dts));
> +
> +        if (st->event_flags & AVSTREAM_EVENT_FLAG_METADATA_UPDATED) {
> +            result = av_dict_get_string(st->metadata, &metadata, '=', ':');
> +            if (result < 0)
> +                goto end;
> +
> +            printf("Stream ID: %d, new metadata: %s\n", pkt->stream_index,
> +                   strlen(metadata) ? metadata : "N/A");
> +
> +            st->event_flags &= ~AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
> +        }
> +

> +        result = avcodec_send_packet(ctx, pkt);
> +        av_packet_unref(pkt);
> +
> +        if (result < 0)
> +            goto end;
> +
> +        result = avcodec_receive_frame(ctx, fr);
> +        if (result == AVERROR_EOF) {
> +            result = 0;
> +            goto end;
> +        }
> +
> +        if (result == AVERROR(EAGAIN))
> +            continue;

This code is not what the API docs suggest.

avcodec_receive_frame() should be called in a loop

there is not a guranteed 1:1 relation between packets and frames
Even if thats the case most of the time

Or is this tool only supposed to work with some specific types
of files ? (if so this should be documented)

thx

[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"Nothing to hide" only works if the folks in power share the values of
you and everyone you know entirely and always will -- Tom Scott


[-- Attachment #2.1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 201 bytes --]

[-- Attachment #2.1.3: ATT00001.txt --]
[-- Type: text/plain, Size: 258 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [TC] "Future Log Output Default"

[Rémi Denis-Courmont]

Le lauantaina 26. huhtikuuta 2025, 18.10.27 Itä-Euroopan kesäaika Michael 
Niedermayer a écrit :
> This is just an announcement that the TC has been asked to look into
>     avutil/log: Add log flag to control printing of memory addresses
>     GitHub:    https://github.com/ffstaging/FFmpeg/pull/59
>     Patchwork:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14094 ...
> 
> and the disagreement between people about it.
> 
> So far, Niklas, Martin and myself have commented, there have been no formal
> decissions and no votes, we just since yesterday send some comments.
> 
> From these to me it seems the TC members who spoke so far seem to agree
> that the addresses in the log are "mostly noise".

That looks like a very ambivalent qualification and it is unclear to me what 
that would actually imply in terms of technical policies.

TBH, I don't see the point in adding a flag for pointers. The kernel does have 
something like that. But there it is meant to avoid leaking information about 
the kernel address space layout, that could be used to defeat ASLR or heap 
randomisation.

In user space programs and libraries, there is typically no lower privileged 
run-time environment running in a different or subset address space, and which 
could access the logs. So the data leakage concern is moot. It could be a 
problem in a program that provides some kind of sandbox environment for 
untrusted, such as web browsers, but FFmpeg has no such thing.

And going back to the Linux kernel case, I do note that:

1) The implementation overhead is vastly with reduced with the usage of custom 
format string specifiers - but FFmpeg probably doesn't want to take that route, 
in which case the feature will necessarily be much more invasive than in 
Linux.

2) It has been a game of whack-a-mole, with printed pointers regularly being 
found or (re)introduced.

3) Any underlying library hooked to FFmpeg logs would have to be audited for 
leaking pointers or pointer-like handles as well.

Seems pretty steep price for questionable gains.

Nevertheless, FFmpeg should of course avoid printing pointers unless there is 
no better alternatives. As you noted, they are mostly useless in logs except 
as temporally unique identifiers.

-- 
德尼-库尔蒙‧雷米
Tapiolan uusi kaupunki, Uudenmaan entinen Suomen tasavalta



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".