Encoding: add encoding types for several enum types #173

nveeser · 2025-11-29T01:20:21Z

Update Rational, PixelFormat and ChannelLayout to implement json interfaces which allow serializing these types to JSON without intermediate parser steps

Also add names for levels so the level name can be printed using fmt.Printf("%s")

nveeser · 2025-11-29T01:33:30Z

My apologies for creating a separate branch / pull request.
I switch from git to jj and organized all the changes into a chain but I deleted a number of branches and re-exporting lost the history and therefore the PR was in a terminal state.

Apologies as the original comments are not here but instead on the original PR.
I went through and tried to resolve all the comments in #161

I will try to copy over open questions here.

nveeser · 2025-11-29T01:36:39Z

channel_layout.go

 }
+
+// https://ffmpeg.org/doxygen/7.0/group__lavu__audio__channels.html#ga16a894cd275eba471ccad4fe1c5e5c28
+func ChannelLayoutFromString(s string) (ChannelLayout, error) {


I don't know the library that well but its seems to be both looking for an existing layout by name and parsing a string which can be used to build a layout struct.

Since its both a "Find" and a "Parse", would it make more sense to use the terminology of the underlying library ChannelLayoutFromString()?

I'll be honest, you make a valid argument, however we've been using the pattern Find[1]By[2] in the rest of the library (e.g. here or here) that's why I'd rather stay consistent and rename it to FindChannelLayoutByName 👍

nveeser · 2025-11-29T01:38:47Z

channel_layout.go

+	cn := C.CString(s)
+	defer C.free(unsafe.Pointer(cn))
+	if err := newError(C.av_channel_layout_from_string(&c, cn)); err != nil {
+		return ChannelLayout{}, fmt.Errorf("astiav: could not find or parse channel layout %q: %w", s, err)


Seemed pretty useful and idiomatic (a la strconv.ParseInt but not json.Marshal) to include the argument in the error value.
Otherwise it would be EINVAL or ENOMEM w/ no logging so there is no other context in the error value to read.

At least in the context of parsing JSON or similar, if you are parsing multiple values in a byte string, getting "invalid" as the only error is going to very difficult to debug.

Is the concern that including the argument string in the error value is too long?

I actually do agree with you 👍

Update Rational, PixelFormat and ChannelLayout to implement json interfaces which allow serializing these types to JSON without intermediate parser steps Also add names for levels

asticode

No worries, I've answered in this new PR 👍

asticode · 2025-11-29T09:43:31Z

channel_layout.go

 }
+
+// https://ffmpeg.org/doxygen/7.0/group__lavu__audio__channels.html#ga16a894cd275eba471ccad4fe1c5e5c28
+func ChannelLayoutFromString(s string) (ChannelLayout, error) {


I'll be honest, you make a valid argument, however we've been using the pattern Find[1]By[2] in the rest of the library (e.g. here or here) that's why I'd rather stay consistent and rename it to FindChannelLayoutByName 👍

asticode · 2025-11-29T09:48:48Z

channel_layout.go

+	cn := C.CString(s)
+	defer C.free(unsafe.Pointer(cn))
+	if err := newError(C.av_channel_layout_from_string(&c, cn)); err != nil {
+		return ChannelLayout{}, fmt.Errorf("astiav: could not find or parse channel layout %q: %w", s, err)


I actually do agree with you 👍

asticode · 2025-11-29T09:49:34Z

channel_layout.go

+	cn := C.CString(s)
+	defer C.free(unsafe.Pointer(cn))
+	if err := newError(C.av_channel_layout_from_string(&c, cn)); err != nil {
+		return ChannelLayout{}, fmt.Errorf("astiav: could not find or parse channel layout %q: %w", s, err)


Could you replace the error message with astiav: channel layout from string %q failed: %w?

asticode · 2025-11-29T09:54:12Z

channel_layout.go

+	}
+	pf, err := ChannelLayoutFromString(s)
+	if err != nil {
+		return err


Could you return this instead fmt.Errorf("astiav: finding channel layout failed: %w", err)?

asticode · 2025-11-29T09:56:05Z

channel_layout_test.go

+		cl3 := ChannelLayout{}
+		require.True(t, cl2.Equal(cl3))
+	})
+	t.Run("ChannelLayoutFromString", func(t *testing.T) {


Could you rename the test to FindChannelLayoutByName?

asticode · 2025-11-29T10:10:20Z

sample_format.go

+	}
+	pf := FindSampleFormat(s)
+	if pf == SampleFormatNone {
+		return fmt.Errorf("astiav: invalid SampleFormat: %q", s)


Can you update the error message to astiav: no sample format with name %q?

asticode · 2025-11-29T10:11:32Z

sample_format.go

+	if s == "" {
+		*f = SampleFormatNone
+	}
+	pf := FindSampleFormat(s)


Can you rename the variable from pf to sf?

asticode · 2025-11-29T10:12:14Z

sample_format.go

 	return C.av_sample_fmt_is_planar((C.enum_AVSampleFormat)(f)) > 0
 }
+
+func FindSampleFormat(name string) SampleFormat {


Can you add tests for all new sample format methods? (you can use something similar to pixel format)

asticode · 2025-11-29T10:12:49Z

sample_format.go

 	return C.av_sample_fmt_is_planar((C.enum_AVSampleFormat)(f)) > 0
 }
+
+func FindSampleFormat(name string) SampleFormat {


Could you rename this to FindSampleFormatByName?

asticode · 2025-11-29T10:22:44Z

format_context_test.go

 	sdp, err := fc1.SDPCreate()
 	require.NoError(t, err)
-	require.Equal(t, "v=0\r\no=- 0 0 IN IP4 127.0.0.1\r\ns=Big Buck Bunny\r\nt=0 0\r\na=tool:libavformat 61.1.100\r\nm=video 0 RTP/AVP 96\r\nb=AS:441\r\na=rtpmap:96 H264/90000\r\na=fmtp:96 packetization-mode=1; sprop-parameter-sets=Z0LADasgKDPz4CIAAAMAAgAAAwBhHihUkA==,aM48gA==; profile-level-id=42C00D\r\na=control:streamid=0\r\nm=audio 0 RTP/AVP 97\r\nb=AS:161\r\na=rtpmap:97 MPEG4-GENERIC/48000/2\r\na=fmtp:97 profile-level-id=1;mode=AAC-hbr;sizelength=13;indexlength=3;indexdeltalength=3; config=1190\r\na=control:streamid=1\r\n", sdp)
+	wants := []string{


I'd rather use the following:

require.Regexp(t, regexp.MustCompile("v=0\r\no=- 0 0 IN IP4 127.0.0.1\r\ns=Big Buck Bunny\r\nt=0 0\r\na=tool:libavformat [\\d]+.[\\d]+.[\\d]+\r\nm=video 0 RTP/AVP 96\r\nb=AS:441\r\na=rtpmap:96 H264/90000\r\na=fmtp:96 packetization-mode=1; sprop-parameter-sets=Z0LADasgKDPz4CIAAAMAAgAAAwBhHihUkA==,aM48gA==; profile-level-id=42C00D\r\na=control:streamid=0\r\nm=audio 0 RTP/AVP 97\r\nb=AS:161\r\na=rtpmap:97 MPEG4-GENERIC/48000/2\r\na=fmtp:97 profile-level-id=1;mode=AAC-hbr;sizelength=13;indexlength=3;indexdeltalength=3; config=1190\r\na=control:streamid=1\r\n"), sdp)

nveeser force-pushed the encoding branch from fdcaa9b to ed25d36 Compare November 29, 2025 01:28

nveeser marked this pull request as draft November 29, 2025 01:29

nveeser closed this Nov 29, 2025

nveeser reopened this Nov 29, 2025

nveeser commented Nov 29, 2025

View reviewed changes

Encoding: add encoding types for several enum types

d75e423

Update Rational, PixelFormat and ChannelLayout to implement json interfaces which allow serializing these types to JSON without intermediate parser steps Also add names for levels

nveeser force-pushed the encoding branch from ed25d36 to d75e423 Compare November 29, 2025 01:49

nveeser marked this pull request as ready for review November 29, 2025 01:51

asticode requested changes Nov 29, 2025

View reviewed changes

Encoding: add encoding types for several enum types #173

Are you sure you want to change the base?

Encoding: add encoding types for several enum types #173

Conversation

nveeser commented Nov 29, 2025

Uh oh!

nveeser commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nveeser Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asticode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nveeser commented Nov 29, 2025 •

edited

Loading

nveeser Nov 29, 2025 •

edited

Loading