Add format selection criteria longside, shortside #30742

dirkf · 2022-03-14T13:18:54Z

Please follow the guide below

You will be asked some questions, please read them carefully and answer honestly
Put an x into all the boxes [ ] relevant to your pull request (like that [x])
Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

Searched the bugtracker for similar pull requests
Read adding new extractor tutorial
Read youtube-dl coding conventions and adjusted the code to meet them
Covered the code with tests (note that PRs without tests will be REJECTED)
Checked the code with flake8

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

I am the original author of this code and I am willing to release it under Unlicense
I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

Bug fix
Improvement
New extractor
New feature

Description of your pull request and other information

Explanation of your pull request in arbitrary form goes here. Please make sure the description explains the purpose and effect of your pull request and is worded well enough to be understood. Provide as much context and examples as possible.

dirkf

@mk-pmb, I've created the PR for you. Do check these review items for which GH won't let me Request Changes from myself. Or you might rather create your own PR.

With my suggested changes the existing test/test_YoutubeDL.py test succeeds. You should either add a new test case for long/short/side, or add something to the test_youtube_format_selection() test case.

youtube_dl/YoutubeDL.py

dirkf · 2022-03-14T13:59:35Z

youtube_dl/YoutubeDL.py

@@ -1484,6 +1491,17 @@ def sanitize_numeric_fields(info):
        sanitize_string_field(info_dict, 'id')
        sanitize_numeric_fields(info_dict)

+        def add_calculated_video_proprties(fmt):
+            dimensions = [fmt.get(side) for side in ('width', 'height',)]


Or in 1 line:

dimensions = filter(None, [fmt.get(side) for side in ('width', 'height',)])

dirkf · 2022-03-14T14:01:34Z

youtube_dl/YoutubeDL.py

+        def add_calculated_video_proprties(fmt):
+            dimensions = [fmt.get(side) for side in ('width', 'height',)]
+            dimensions = [n for n in dimensions if n is not None]
+            if len(dimensions):


You need both width and height to be set, otherwise long and short are indeterminate, as shown by the test error on l.1499 when trying to evaluate min(1000) ('int' object is not iterable). So:

if len(dimensions) == 2:

The doc for min():

min(iterable[, key]) min(arg1, arg2, *args[, key]) ...

Because of the optional key, parameter, the reasonable result min(x) -> x isn't possible. If you don't know the length of the list whose min (resp. max) is being found, you have to say min(iter(dimensions)) rather than *dimensions.

Sorry for being late. It seems I missed a notification.
Thanks for teaching me! I hope I'll remember this trap.
The iter() approach seemed best, but min() and max() throw on an empty, list, and I'd prefer to be tolerant for bad input, as in just giving 0 as the length for non-dimensional video. ~~Thus, in my next version I'll propose min(0, 0, *dimensions) unconditionally.~~

Edit: Yeah I see how this fails for min(). :D

dirkf · 2022-03-14T14:06:16Z

youtube_dl/YoutubeDL.py

+                    'shortside': min(*dimensions),
+                    'longside': max(*dimensions),
+                })
+        for fmt in info_dict['formats']:


Not all info_dicts have a formats key (see extractor/common.py). In case the dimensions have been set in the top level, you could process the info_dict as a format:

for fmt in info_dict.get('formats') or [info_dict]:

dirkf · 2022-03-14T14:09:10Z

youtube_dl/YoutubeDL.py

@@ -334,6 +334,11 @@ class YoutubeDL(object):
        'playlist_index',
    ))

+    _NUMERIC_FIELDS |= set(('longside', 'shortside',))


If we're doing this, I'd just have these on a separate line in the original list:

'width', 'height', 'tbr', 'abr', 'asr', 'vbr', 'fps', 'filesize', 'filesize_approx', 'longside', 'shortside', 'timestamp', 'upload_year', 'upload_month', 'upload_day',

As suggested in my refactor idea, IMO this list should not exist in the first place; rather, its values should be collected from the sections in code that implement them. I will not oppose anyone who decides to enact your solution, but I won't do anything that may look like approval.

mk-pmb · 2022-03-14T16:11:37Z

Thank you for the kind gesture. I purposefully did not file a PR because my patch is not worthy of being merged into the main codebase, and never was intended to.

My patch was an attempt to provide a stopgap at a time when the idea of my feature request was met with strong resistance. Someone who seemed to be a team member argued that this feature should not exist in yt-dl, so my patch was aimed at helping users merge it locally into their systems.

Now if maintainers decide that the feature is worth adding, we should try and find someone who knows how to implement it cleanly and elegantly.

I recommend to add the feature, but to not use my patch for it.

dirkf · 2022-03-14T18:02:54Z

The status of a commenter should be shown on the right-hand side of the message header.

Your patch is in line with the existing code structure. The fields in the NUMERIC_FIELDS list are separately documented. I don't think it would help to generate the list dynamically. That's why the code is as it is (as far as I know and without a lot of blame-seeking).

If this feature is to be implemented, the approach used in your patch seems fine. However it does need test(s) as I mentioned, and also updating README.md.

A proposal for refactoring the format selection code ought to be a separate PR, also possibly acquiring some other recent and proposed changes from yt-dlp.

mk-pmb · 2024-01-11T01:09:19Z

A proposal for refactoring the format selection code ought to be a separate PR,

Nowadays I accept that argument and have updated my code to your suggestions.

You should either add a new test case for long/short/side, or add something to the test_youtube_format_selection() test case.

Might take a while, so anyone else reading this, feel free to help out!

Edit: I just discovered that I already had some tests in #30744, but accidentially closed that thread. I rebased that branch. Should I just merge them here, or make a new PR for them so we can first check if the tests would work?

dirkf · 2024-01-23T02:40:09Z

Please update this PR if you can, or start a replacement if not. Thanks.

mk-pmb · 2024-01-23T03:58:58Z

Done. I'm optimistic. Someone should look at the :TODO: marks in the tests though.

mk-pmb · 2024-05-16T00:47:17Z

I reviewed this again. The first :TODO: is just an API question: Can we expect process_ie_result to not mangle its arguments?

    ydl.process_ie_result(info_dict.copy())
    # ^-- :TODO: Do we need this .copy()?

The other :TODO: is a bug outside the scope of this PR and will be solved by #32786.
I amended this PR to include that fix and omit both :TODO: lines so you can easily merge if you don't care about potential performance savings on the dict copy.

…r both (ytdl-org#30737)

mk-pmb · 2024-05-16T01:07:57Z

I don't understand the CI errors.

AssertionError: Regexp didn't match: u'(?:(?:#.*?|\\s*)\\n)*from __future__ import
  (?:[a-z_]+,\\s*)*unicode_literals' not found, unicode_literals import missing in
  /…/test/formatselection/criteria_longside_shortside.py

but from __future__ import unicode_literals is obviosuly there, in exactly that file, line 5.

dirkf commented Mar 14, 2022

View reviewed changes

dirkf marked this pull request as draft March 14, 2022 14:29

dirkf mentioned this pull request Mar 14, 2022

Feature: Format filter: Rotation-independent measurements longside and shortside to help with portrait video #30737

Open

3 tasks

dirkf force-pushed the master branch from 01bf89e to 4c6fba3 Compare August 26, 2022 07:51

mk-pmb deleted the fmtsel-longside-shortside branch January 10, 2024 23:41

mk-pmb restored the fmtsel-longside-shortside branch January 10, 2024 23:41

mk-pmb force-pushed the fmtsel-longside-shortside branch from 31af229 to 5a2aca1 Compare January 11, 2024 01:06

dirkf assigned mk-pmb Jan 23, 2024

mk-pmb force-pushed the fmtsel-longside-shortside branch 3 times, most recently from e685543 to 69872de Compare January 23, 2024 03:58

mk-pmb force-pushed the fmtsel-longside-shortside branch 2 times, most recently from fe34885 to bfc5707 Compare May 16, 2024 00:46

mk-pmb force-pushed the fmtsel-longside-shortside branch from bfc5707 to 8acf85a Compare May 16, 2024 00:48

[core] Empty format selection string means anything goes.

ce031e9

mk-pmb force-pushed the fmtsel-longside-shortside branch from 8acf85a to d4f8a34 Compare May 16, 2024 00:54

[core] Add format selection criteria longside/shortside, and tests fo…

0c1db86

…r both (ytdl-org#30737)

mk-pmb force-pushed the fmtsel-longside-shortside branch from d4f8a34 to 0c1db86 Compare May 16, 2024 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add format selection criteria longside, shortside #30742

Add format selection criteria longside, shortside #30742

dirkf commented Mar 14, 2022 •

edited

dirkf left a comment •

edited

dirkf Mar 14, 2022

dirkf Mar 14, 2022 •

edited

mk-pmb Jan 11, 2024 •

edited

dirkf Mar 14, 2022

dirkf Mar 14, 2022

mk-pmb Mar 14, 2022

mk-pmb commented Mar 14, 2022

dirkf commented Mar 14, 2022 •

edited

mk-pmb commented Jan 11, 2024 •

edited

dirkf commented Jan 23, 2024

mk-pmb commented Jan 23, 2024 •

edited

mk-pmb commented May 16, 2024

mk-pmb commented May 16, 2024

Add format selection criteria longside, shortside #30742

Are you sure you want to change the base?

Add format selection criteria longside, shortside #30742

Conversation

dirkf commented Mar 14, 2022 • edited

Please follow the guide below

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

What is the purpose of your pull request?

Description of your pull request and other information

dirkf left a comment • edited

Choose a reason for hiding this comment

dirkf Mar 14, 2022

Choose a reason for hiding this comment

dirkf Mar 14, 2022 • edited

Choose a reason for hiding this comment

mk-pmb Jan 11, 2024 • edited

Choose a reason for hiding this comment

dirkf Mar 14, 2022

Choose a reason for hiding this comment

dirkf Mar 14, 2022

Choose a reason for hiding this comment

mk-pmb Mar 14, 2022

Choose a reason for hiding this comment

mk-pmb commented Mar 14, 2022

dirkf commented Mar 14, 2022 • edited

mk-pmb commented Jan 11, 2024 • edited

dirkf commented Jan 23, 2024

mk-pmb commented Jan 23, 2024 • edited

mk-pmb commented May 16, 2024

mk-pmb commented May 16, 2024

dirkf commented Mar 14, 2022 •

edited

dirkf left a comment •

edited

dirkf Mar 14, 2022 •

edited

mk-pmb Jan 11, 2024 •

edited

dirkf commented Mar 14, 2022 •

edited

mk-pmb commented Jan 11, 2024 •

edited

mk-pmb commented Jan 23, 2024 •

edited