-
Notifications
You must be signed in to change notification settings - Fork 4
avutil/log: Add log flag to control printing of memory addresses #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
On the FFmpeg mailing list, Nicolas George wrote (reply to this):
|
User |
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
User |
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Gyan Doshi wrote (reply to this):
|
User |
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Gyan Doshi wrote (reply to this):
|
e686825
to
411c77b
Compare
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
On the FFmpeg mailing list, Nicolas George wrote (reply to this):
|
On the FFmpeg mailing list, Nicolas George wrote (reply to this):
|
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Nicolas George wrote (reply to this):
|
User |
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
User |
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Marvin Scholz wrote (reply to this):
|
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Marvin Scholz wrote (reply to this):
|
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
On the FFmpeg mailing list, Marvin Scholz wrote (reply to this):
|
On the FFmpeg mailing list, Soft Works wrote (reply to this):
|
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
ea2a970
to
6d3f305
Compare
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
6d3f305
to
22c5189
Compare
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
60ceab0
to
bfe2e51
Compare
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
bfe2e51
to
dc334f3
Compare
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
fftools/ffmpeg.c
Outdated
@@ -954,7 +954,7 @@ int main(int argc, char **argv) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, Nicolas George wrote (reply to this):
softworkz (HE12025-04-10):
> From: softworkz <[email protected]>
>
> This commit adds the mem log flag.
> When specifying this flag at the command line, context prefixes will
> be printed with memory addresses like in earlier ffmpeg versions.
>
> Example with mem flag:
>
> [hevc @ 0000018e72a89cc0] .....
As explained recently, strong opposition to this being the default.
--
Nicolas George
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
> -----Original Message-----
> From: Nicolas George <[email protected]>
> Sent: Donnerstag, 10. April 2025 08:51
> To: FFmpeg development discussions and patches <[email protected]>
> Cc: softworkz <[email protected]>
> Subject: Re: [FFmpeg-devel] [PATCH v10 2/3] fftools: add mem log flag
> and disable printing addresses by default
>
> softworkz (HE12025-04-10):
> > From: softworkz <[email protected]>
> >
> > This commit adds the mem log flag.
> > When specifying this flag at the command line, context prefixes will
> > be printed with memory addresses like in earlier ffmpeg versions.
> >
> > Example with mem flag:
> >
> > [hevc @ 0000018e72a89cc0] .....
>
> As explained recently, strong opposition to this being the default.
>
> --
> Nicolas George
Hi Nicolas,
other than your previous comments, questioning the default behavior is an acceptable concern.
I think in this case it makes sense to just put it up for a vote.
It's a significant change which should be backed by a majority opinion.
Best,
sw
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, Michael Niedermayer wrote (reply to this):
--===============1685350178670096472==
Content-Type: multipart/signed; micalg=pgp-sha512;
protocol="application/pgp-signature"; boundary="KLWx2LEuultqqEB2"
Content-Disposition: inline
--KLWx2LEuultqqEB2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Hi
On Thu, Apr 10, 2025 at 08:51:04AM +0200, Nicolas George wrote:
> softworkz (HE12025-04-10):
> > From: softworkz <[email protected]>
> >=20
> > This commit adds the mem log flag.
> > When specifying this flag at the command line, context prefixes will
> > be printed with memory addresses like in earlier ffmpeg versions.
> >=20
> > Example with mem flag:
> >=20
> > [hevc @ 0000018e72a89cc0] .....
>=20
> As explained recently, strong opposition to this being the default.
just some random comments:
I think some way to distingish two different "hevc" instances
with high probability should remain.
About the addresses. Iam curious how frequently do people use them ?
and for what exactly ?
I do think *item_name() should be used more often. The "hevc" is a
quite bland identifcation of the instance.
in absence of a item_name(), that is *av_default_item_name()
which prints just the class name. I think printing the address by default
is reasonable otherwsie instances would be always indistingishable
beyond that, i dont remember using the addresses and would not
mind if it gets replaced by something more usefull more repeatable
with maybe some mem flag that could force them to be printed in all
cases
but i dont know, really depends on what the community prefers
thx
[...]
--=20
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
--KLWx2LEuultqqEB2
Content-Type: application/pgp-signature; name="signature.asc"
-----BEGIN PGP SIGNATURE-----
iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZ/+z8wAKCRBhHseHBAsP
q8QrAJ9qIm0dnTwuFQ6y+TTVYtSFbagwJQCeNnoKY0DFYf/rLZb4VJ3c0szwEpU=
=ineg
-----END PGP SIGNATURE-----
--KLWx2LEuultqqEB2--
--===============1685350178670096472==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
--===============1685350178670096472==--
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, Nicolas George wrote (reply to this):
Michael Niedermayer (HE12025-04-16):
> I think some way to distingish two different "hevc" instances
> with high probability should remain.
This is exactly my point. The address is meaningless�, but it is a sure
way to distinguish instances of the same components without tons of
fragile code.
1: In a debugger, the address would be available without printing.
> I do think *item_name() should be used more often.
Making sure it returns something different for different instances
without the code getting complex and/or the name getting long is quite a
challenge.
This fruit is neither low-hanging nor tasty. My position on this: leave
the address alone unless one has a very good idea to achieve the goal.
Regards,
--
Nicolas George
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, Gyan Doshi wrote (reply to this):
On 2025-04-16 07:20 pm, Nicolas George wrote:
> Michael Niedermayer (HE12025-04-16):
>> I think some way to distingish two different "hevc" instances
>> with high probability should remain.
> This is exactly my point. The address is meaningless¹, but it is a sure
> way to distinguish instances of the same components without tons of
> fragile code.
For CLI users, the fftool log prefixes already identify the input file
and the stream in many msgs, e.g.
e.g. [vist#0:0/h264 @ 000002485a204540] [dec:h264 @ 000002485a36e900]
Decoder thread received EOF packet
So, an opt-in flag should be added even if it's not made the default, or
no UUID replacement is found for the memaddr.
Regards,
Gyan
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of
> Michael Niedermayer
> Sent: Mittwoch, 16. April 2025 15:43
> To: FFmpeg development discussions and patches <ffmpeg-
> [email protected]>
> Subject: Re: [FFmpeg-devel] [PATCH v10 2/3] fftools: add mem log flag
> and disable printing addresses by default
>
> Hi
>
> On Thu, Apr 10, 2025 at 08:51:04AM +0200, Nicolas George wrote:
> > softworkz (HE12025-04-10):
> > > From: softworkz <[email protected]>
> > >
> > > This commit adds the mem log flag.
> > > When specifying this flag at the command line, context prefixes
> will
> > > be printed with memory addresses like in earlier ffmpeg versions.
> > >
> > > Example with mem flag:
> > >
> > > [hevc @ 0000018e72a89cc0] .....
> >
> > As explained recently, strong opposition to this being the default.
>
> just some random comments:
>
> I think some way to distingish two different "hevc" instances
> with high probability should remain.
>
> About the addresses. Iam curious how frequently do people use them ?
> and for what exactly ?
>
> I do think *item_name() should be used more often. The "hevc" is a
> quite bland identifcation of the instance.
>
> in absence of a item_name(), that is *av_default_item_name()
> which prints just the class name. I think printing the address by
> default
> is reasonable otherwsie instances would be always indistingishable
>
> beyond that, i dont remember using the addresses and would not
> mind if it gets replaced by something more usefull more repeatable
> with maybe some mem flag that could force them to be printed in all
> cases
>
> but i dont know, really depends on what the community prefers
Thanks Michael,
I'm gonna keep myself out of arguing, I believe that this is a decision
that should be based on what the community (majority) will prefer.
I just want to summarize the different implementations that are available
already while the patchset has walked its way through the various comments
that were made.
-----------------------------------------
1. Implementation in avutil with replacement Ids
The replacement Ids are a direct projection of the mem addresses, which
means that they are equally evident - yet subject to the same limitations
as the mem-addresses.
But other than the mem-addresses, this creates the identical log output
when repeating the same command.
Reviews:
- It has been criticized that this introduces global state which is
undesired in case of library use
- It has been suggested to implement this in fftools only, in a way that
fftools has its own and independent logging implementation, no longer
using the one in avutil. It was also based on the idea that this would
allow to do other things in the future which cannot reasonably be
implemented in avutil
This has led to the second variant:
-----------------------------------------
2. Implementation in fftools with replacement Ids
Replacement Id behavior is the same as in (1).
Obviously, there's no point in writing this from scratch, so the starting
point was the logging code from avutil
Reviews:
- This has raised criticism due to the copied logging code
- It was suggested to drop the replacement Ids and then it could be
implemented in avutil only
This has led to the third version:
-----------------------------------------
3. Implementation back in avutil without Ids
Means you switch the mem addresses on or off, when off, no Ids are printed
-----------------------------------------
Best regards,
softworkz
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
@@ -1,5 +1,8 @@ | |||
The last version increases of all libraries were on 2025-03-28 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, Andreas Rheinhardt wrote (reply to this):
softworkz:
> From: softworkz <[email protected]>
>
> which is controls prefix formatting. With this flag set, the prefix is
> printed without the memory address, otherwise it is included.
>
> Signed-off-by: softworkz <[email protected]>
> ---
> doc/APIchanges | 3 +++
> libavutil/log.c | 6 ++++--
> libavutil/log.h | 5 +++++
> libavutil/version.h | 2 +-
> 4 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 65bf5a9419..db832f8b19 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -1,5 +1,8 @@
> The last version increases of all libraries were on 2025-03-28
>
> +2025-03-xx - xxxxxxxxxx - lavu 60.2.100 - log.h
> + Add flag AV_LOG_NO_PRINT_MEMADDRESS
> +
> API changes, most recent first:
>
> 2025-04-07 - 19e9a203b7 - lavu 60.01.100 - dict.h
> diff --git a/libavutil/log.c b/libavutil/log.c
> index c5ee876a88..1949a797e7 100644
> --- a/libavutil/log.c
> +++ b/libavutil/log.c
> @@ -327,16 +327,18 @@ static void format_line(void *avcl, int level, const char *fmt, va_list vl,
>
> if(type) type[0] = type[1] = AV_CLASS_CATEGORY_NA + 16;
> if (*print_prefix && avc) {
> + const char *p_fmt = flags & AV_LOG_NO_PRINT_MEMADDRESS ? "[%s] " : "[%s @ %p] ";
> +
> if (avc->parent_log_context_offset) {
> AVClass** parent = *(AVClass ***) (((uint8_t *) avcl) +
> avc->parent_log_context_offset);
> if (parent && *parent) {
> - av_bprintf(part+0, "[%s @ %p] ",
> + av_bprintf(part+0, p_fmt,
> item_name(parent, *parent), parent);
> if(type) type[0] = get_category(parent);
> }
> }
> - av_bprintf(part+1, "[%s @ %p] ",
> + av_bprintf(part+1, p_fmt,
> item_name(avcl, avc), avcl);
> if(type) type[1] = get_category(avcl);
> }
> diff --git a/libavutil/log.h b/libavutil/log.h
> index dd094307ce..499c5d71ab 100644
> --- a/libavutil/log.h
> +++ b/libavutil/log.h
> @@ -416,6 +416,11 @@ int av_log_format_line2(void *ptr, int level, const char *fmt, va_list vl,
> */
> #define AV_LOG_PRINT_DATETIME 8
>
> +/**
> + * Do not print memory addresses of context instances.
> + */
> +#define AV_LOG_NO_PRINT_MEMADDRESS 16
> +
> void av_log_set_flags(int arg);
> int av_log_get_flags(void);
>
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 5139883569..4717cd562b 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
> */
>
> #define LIBAVUTIL_VERSION_MAJOR 60
> -#define LIBAVUTIL_VERSION_MINOR 1
> +#define LIBAVUTIL_VERSION_MINOR 2
> #define LIBAVUTIL_VERSION_MICRO 100
>
> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
The commit message needs an update.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
> -----Original Message-----
> From: ffmpeg-devel <[email protected]> On Behalf Of
> Andreas Rheinhardt
> Sent: Donnerstag, 10. April 2025 09:39
> To: [email protected]
> Subject: Re: [FFmpeg-devel] [PATCH v10 1/3] avutil/log: Add log flag
> AV_LOG_PRINT_MEMADDRESSES
>
> softworkz:
> > From: softworkz <[email protected]>
> >
> > which is controls prefix formatting. With this flag set, the prefix is
> > printed without the memory address, otherwise it is included.
> >
> > Signed-off-by: softworkz <[email protected]>
> > ---
> > doc/APIchanges | 3 +++
> > libavutil/log.c | 6 ++++--
> > libavutil/log.h | 5 +++++
> > libavutil/version.h | 2 +-
> > 4 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index 65bf5a9419..db832f8b19 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -1,5 +1,8 @@
> > The last version increases of all libraries were on 2025-03-28
> >
> > +2025-03-xx - xxxxxxxxxx - lavu 60.2.100 - log.h
> > + Add flag AV_LOG_NO_PRINT_MEMADDRESS
> > +
> > API changes, most recent first:
> >
> > 2025-04-07 - 19e9a203b7 - lavu 60.01.100 - dict.h
> > diff --git a/libavutil/log.c b/libavutil/log.c
> > index c5ee876a88..1949a797e7 100644
> > --- a/libavutil/log.c
> > +++ b/libavutil/log.c
> > @@ -327,16 +327,18 @@ static void format_line(void *avcl, int level,
> const char *fmt, va_list vl,
> >
> > if(type) type[0] = type[1] = AV_CLASS_CATEGORY_NA + 16;
> > if (*print_prefix && avc) {
> > + const char *p_fmt = flags & AV_LOG_NO_PRINT_MEMADDRESS ?
> "[%s] " : "[%s @ %p] ";
> > +
> > if (avc->parent_log_context_offset) {
> > AVClass** parent = *(AVClass ***) (((uint8_t *) avcl) +
> > avc->parent_log_context_offset);
> > if (parent && *parent) {
> > - av_bprintf(part+0, "[%s @ %p] ",
> > + av_bprintf(part+0, p_fmt,
> > item_name(parent, *parent), parent);
> > if(type) type[0] = get_category(parent);
> > }
> > }
> > - av_bprintf(part+1, "[%s @ %p] ",
> > + av_bprintf(part+1, p_fmt,
> > item_name(avcl, avc), avcl);
> > if(type) type[1] = get_category(avcl);
> > }
> > diff --git a/libavutil/log.h b/libavutil/log.h
> > index dd094307ce..499c5d71ab 100644
> > --- a/libavutil/log.h
> > +++ b/libavutil/log.h
> > @@ -416,6 +416,11 @@ int av_log_format_line2(void *ptr, int level,
> const char *fmt, va_list vl,
> > */
> > #define AV_LOG_PRINT_DATETIME 8
> >
> > +/**
> > + * Do not print memory addresses of context instances.
> > + */
> > +#define AV_LOG_NO_PRINT_MEMADDRESS 16
> > +
> > void av_log_set_flags(int arg);
> > int av_log_get_flags(void);
> >
> > diff --git a/libavutil/version.h b/libavutil/version.h
> > index 5139883569..4717cd562b 100644
> > --- a/libavutil/version.h
> > +++ b/libavutil/version.h
> > @@ -79,7 +79,7 @@
> > */
> >
> > #define LIBAVUTIL_VERSION_MAJOR 60
> > -#define LIBAVUTIL_VERSION_MINOR 1
> > +#define LIBAVUTIL_VERSION_MINOR 2
> > #define LIBAVUTIL_VERSION_MICRO 100
> >
> > #define LIBAVUTIL_VERSION_INT
> AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>
> The commit message needs an update.
>
> - Andreas
>
> _______________________________________________
Thanks! Fixed locally.
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
298e9ea
to
0651d3b
Compare
…y default Memory addresses are no longer printed by default. With this flag set, memory addresses are included like in earlier versions. Signed-off-by: softworkz <[email protected]>
This commit adds the mem log flag. When specifying this flag at the command line, context prefixes will be printed with memory addresses like in earlier ffmpeg versions. Example with mem flag: [hevc @ 0000018e72a89cc0] ..... without (new behavior): [hevc] ..... Signed-off-by: softworkz <[email protected]>
Signed-off-by: softworkz <[email protected]>
0651d3b
to
fd233fb
Compare
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
On the FFmpeg mailing list, Kieran Kunhya via ffmpeg-devel wrote (reply to this):
|
On the FFmpeg mailing list, "softworkz ." wrote (reply to this):
|
--and disable by default in fftools. The benefits are:
without almost every line being different due to those addresses
Before
[hevc @ 0000018e72a89cc0] nal_unit_type:
[hevc @ 0000018e72a89cc0] Decoding PPS
[hevc @ 0000018e72a89cc0] nal_unit_type: 39(SEI_P..
[hevc @ 0000018e72a89cc0] Decoding SEI
[mp4 @ 0000018e72a8e240] All
[mp4 @ 0000018e72a8e240] Afte
[hevc @ 0000018e742f6b40] Decoded frame with POC ..
detected 16 logical cores
[Parsed_scale_0 @ 0000018e74382f40] Setting 'w' t..
[Parsed_scale_0 @ 0000018e74382f40] Setting 'h' t..
[Parsed_scale_1 @ 0000018e74382440] Setting 'w' t..
[mjpeg @ 0000018e743210c0] Forcing thread count t..
[mjpeg @ 0000018e743210c0] intra_quant_bias = 96
After
[hevc] nal_unit_type:
[hevc] Decoding PPS
[hevc] nal_unit_type: 39(SEI_P..
[hevc] Decoding SEI
[mp4] All info found
[mp4] After avformat_find_
[hevc] Decoded frame with POC 2.
[Parsed_scale_0] Setting 'w' t..
[Parsed_scale_0] Setting 'h' t..
[Parsed_scale_1] Setting 'w' t..
[mjpeg] Forcing thread count t..
[mjpeg] intra_quant_bias = 96
Versions
V2
(as requested by Gyan)
V3
V4
(as suggested by Hendrik Leppkes)
V5
(thanks, Michael)
V6
(as figured to be most likely what Nicolas George was alluding to)
V7
V8
(thanks, Andreas!)
V9
(as suggested by Gyan)
V10
(thanks, Gyan)
V11
.