GHC 2020-06-07

9 comments.

, https://git.io/JfMNA in zmwangx/rust-ffmpeg
No problem!

, https://git.io/JfMbO in zmwangx/rust-ffmpeg
Assuming this is Ubuntu 18.04: it ships a very old version of FFmpeg: https://packages.ubuntu.com/bionic/ffmpeg 3.4.6 to be exact. There's a feature table on README, and as you can see `ffmpeg42` is enabled by default, while you're targeting FFmpeg <4, so you have to disable that:

```
cargo build --no-default-features --features=<other features you need here>
```

Here's a Makefile snippet from another project of mine depending on this crate that does version detection:

```Makefile
LIBAVCODEC_VERSION=$(shell pkg-config --modversion libavcodec)
$(info detected lavc $(LIBAVCODEC_VERSION))
LIBAVCODEC_VERSION_MAJOR := $(word 1,$(subst ., ,$(LIBAVCODEC_VERSION)))
LIBAVCODEC_VERSION_MINOR := $(word 2,$(subst ., ,$(LIBAVCODEC_VERSION)))
ifeq ($(LIBAVCODEC_VERSION_MAJOR),)
  $(error cannot determine libavcodec version with pkg-config)
else ifeq ($(shell test $(LIBAVCODEC_VERSION_MAJOR) -gt 58; echo $$?),0)
  $(warning unknown libavcodec version, possibly from FFmpeg >4; use at own risk)
  FEATURES += ffmpeg42
else ifeq ($(LIBAVCODEC_VERSION_MAJOR),58)
  ifeq ($(shell test $(LIBAVCODEC_VERSION_MINOR) -ge 54; echo $$?),0)
    FEATURES += ffmpeg42
  else ifeq ($(shell test $(LIBAVCODEC_VERSION_MINOR) -ge 35; echo $$?),0)
    FEATURES += ffmpeg41
  else
    FEATURES += ffmpeg4
  endif
endif

CARGO_BUILD_FLAGS += --no-default-features
ifneq ($(FEATURES),)
  $(info enabled features: $(FEATURES))
  CARGO_BUILD_FLAGS += --features "$(FEATURES)"
endif
```

Building for win32 is another story entirely, see #7. The version detection part should follow the same principles.

, https://git.io/JfMb3 in zmwangx/rust-ffmpeg
Thanks for the info!

, https://git.io/JfMbs in zmwangx/rust-ffmpeg
Need better docs on how to build
================================


, https://git.io/JfMbG in zmwangx/rust-ffmpeg
```
apt install -y clang pkg-config libavcodec-dev libavdevice-dev libavfilter-dev libavformat-dev libavresample-dev libavutil-dev libswscale-dev libswresample-dev
```

should probably be enough.

, https://git.io/JfMbZ in zmwangx/rust-ffmpeg
Looks like you don't have `pkg-config` installed. What OS is this? Usually the code should be 127 when the command is not found.

, https://git.io/JfMij in zmwangx/rust-ffmpeg
I didn’t even know pkg-config could run on Windows... Glad it’s working now, I should give it a try myself some time. Btw, are you using a windows-msvc toolchain or a windows-gnu toolchain?

, https://git.io/JfMBq in zmwangx/caterpillar
> I asked a pull request mainly because I thought it was the only way to distribute the modified code with pip to final users; 

Btw, just to be clear, I wasn't discouraging PRs, trying to upstream changes shows respect for an OSS project and I'm grateful for that (of course as I said scoped PRs would be a lot better execution-wise). Just saying you don't have to wait unnecessarily since I can't really promise anything (the other day I finally pushed a fix to an issue I promised to fix precisely a year ago in another project so you get the idea...)

, https://git.io/JfMBm in zmwangx/caterpillar
Thanks for making an effort.

> I didn't

Ah that's what I dreaded. I can't merge all the changes in one commit even if I want to accept all of them verbatim... That would be terrible for blaming and bisecting in the future.

However, your effort isn't in vain, at least it's been made easier for me to understand the changes you want to see, so I think I'll treat them as feature requests instead and make requested changes separately.

Some thoughts:

> Split main() into make_arguments() __launch() __handle_arguments()

Sure, could refactor out the parser. But I definitely won't use dunder unless it's absolutely necessary somehow (it never is).

> Add ffmpeg_loglevel

It's nice to be able to suppress logging, but it's not that simple. Critical functionality actually relies on log level being at least `info`: https://github.com/zmwangx/caterpillar/blob/f7c47e6f3b86b72b7ad461e07863566da5a762af/src/caterpillar/merge.py#L43-L74 so introducing an option like this that only affects one invocation (the final merge pass) but not all others is deceptive and confusing. I'd much rather select the log level automatically based on the existing verbosity level (controlled by repeatable `-q` and `-v`, as is standard), in fact I'm already suppressing logs based on that (see https://github.com/zmwangx/caterpillar/blob/f7c47e6f3b86b72b7ad461e07863566da5a762af/src/caterpillar/merge.py#L75-L79). So I'll do that if I can't think of any reason not to.

> progress_hooks

I can add that, but your reporting is too elaborate for my taste. I'll implement a simpler reporting scheme instead, other logic should be in the caller as they see fit.

> Add invoke() to launch with arguments from external code

Sure, I can add an API like that.

> Add download_m3u8_playlist_or_variant: use the first variant stream found, if a variant streams playlist has been found

That's a useful addition. The nondescriptive `remote0.m3u8` and `remote1.m3u8` are a bit weird though, so I'll likely have to massage the implementation a bit.

---

To summarize, I'll probably implement these changes separately, but it's hard to give you committer credit when I'm not just fixing up minor things in your commits and signing them off. This makes me feel bad since I want to give you credit. So in the future, please consider scoping your commits as well as PRs (a series of PRs for unrelated things would make it a lot easier to accept or reject some).

I'll probably be able to find some time for this either today or within the next week, then I'll cut a release.