Skip to content
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

[BUG] Playlist index numbering is incorrect when using '--playlist-items' #20294

Closed
6 of 9 tasks
CollinChaffin opened this issue Mar 9, 2019 · 11 comments
Closed
6 of 9 tasks

Comments

@CollinChaffin
Copy link

Make sure you are using the latest version: run youtube-dl --version and ensure your version is 2019.03.09. If it's not, read this FAQ entry and update. Issues with outdated version will be rejected.

  • I've verified and I assure that I'm running youtube-dl 2019.03.09

Before submitting an issue make sure you have:

  • At least skimmed through the README, most notably the FAQ and BUGS sections
  • Searched the bugtracker for similar issues including closed ones
  • Checked that provided video/audio/playlist URLs (if any) are alive and playable in a browser

What is the purpose of your issue?

  • Bug report (encountered problems with youtube-dl)
  • Site support request (request for adding support for a new site)
  • Feature request (request for a new functionality)
  • Question
  • Other

Description of your issue, suggested solution and other information

I have read both open and closed issues related to this and don't think I've missed the answer but if so and it is duplicate I apologize ahead of time.

Simple scenario, when downloading a playlist of say, 96 items, and after successfully downloading item 80 a communication/internet failure occurs, ytdl (properly) fails at item 81 and stops processing further items.

For the scenario, here are the video playlist numbers/titles at the point of interest while using this template output -o "%(playlist_index)s - %(title)s.%(ext)s":

  • 79 - Introduction <--DOWNLOADED
  • 80 - Chapter 1 <--DOWNLOADED
  • 81 - Chapter 2 <--FAILED
  • 82 - Chapter 3 <--NOT ATTEMPTED

After communication issue resolved and partial file 81 deleted, the same download command is issued with the following added on the command to allow smooth continuation of the playlist remaining items: --no-overwrites --playlist-items 81-96

Functionality-wise, the remaining 15 playlist items beginning properly with item 81 do in fact download, but with one major difference. Using the same "%(playlist_index)s" as the initial run renders the very unwanted effect on the filenames in the second run of the remaining 15 items:

  • 79 - Introduction
  • 80 - Chapter 1
    --
    <--SECOND RUN STARTS HERE-->
  • 1 - Chapter 2
  • 2 - Chapter 3
  • ....continues to item 15 and not 96 as expected

So, the question is, can you consider adding a feature-request for a switch that changes this behavior and in the above scenario, despite the second run having 15 playlist ITEMS in it, base the "%(playlist_index)s (or perhaps a new variable) on the OVERALL playlist index number, which in the above scenario would have grabbed the remaining 15 items, but instead started their index-numbering at 81 ending at 96 which would reflect the actual playlist number?

Thanks for the consideration and hard work!

@remitamine
Copy link
Collaborator

Duplicate of #10591.

@CollinChaffin
Copy link
Author

CollinChaffin commented Mar 9, 2019

Doh! Yes I swear I did search apparently not very well. :hangshead: Well my darn description was at least prettier! LOL

What I don't understand from that issue however is that it was acknowledged as a BUG two and a half YEARS ago and I see a commit at the same time was it never rolled into production or ? I'm confused why it's locked, open as a bug, looks like there was a fix, etc. or am I reading it wrong?

@remitamine can you look into why this is in this status open with a fix not implemented after years? TIA!

@CollinChaffin
Copy link
Author

@remitamine this doesn't look good clearly this must have just been overlooked seeing this many force pushes and now I do see the ongoing merge conflict:

7bbb700
@yan12125 yan12125 referenced this pull request December 9, 2016 10:34 AM
 Closed
output template playlist_index issue #11339
0 of 4 tasks complete 
@dstftw dstftw force-pushed the rg3:master branch from fa77986 to 0c7a631 June 24, 2017 5:03 PM
@dstftw dstftw force-pushed the rg3:master branch from 4991699 to 1141e91 August 4, 2017 7:42 PM
@dstftw dstftw force-pushed the rg3:master branch to af0f742 October 11, 2017 11:48 AM
@dstftw dstftw force-pushed the rg3:master branch from 37318e1 to 65220c3 January 27, 2018 4:49 PM
@dstftw dstftw force-pushed the rg3:master branch from 8d14fa1 to 5399ab3 February 3, 2018 6:56 PM
@dstftw dstftw force-pushed the rg3:master branch from c486aa9 to 5ee7ae5 December 9, 2018 9:38 AM
@dstftw dstftw force-pushed the rg3:master branch from d99bab0 to e118a87 January 23, 2019 12:40 PM

With as little as this code change was and from initial tests here DOES fix the issue, is there a reason it can't be queued up for an upcoming merge?

@remitamine
Copy link
Collaborator

I see a commit at the same time was it never rolled into production

the commit is part of a Pull Request that hasn't been merged.

@remitamine
Copy link
Collaborator

is there a reason it can't be queued up for an upcoming merge?

the code needs to be tested and check that it doesn't break anything before merging it.

@CollinChaffin
Copy link
Author

CollinChaffin commented Mar 9, 2019

But it's been almost THREE YEARS and it's TEN lines added and two removed. I asked because it looks like it was just overlooked and I just tested and it appears to merge just fine with today's code so could you maybe try to push it into the active queue? The numbering is just broken as you can see, and these 10 lines fix it and if you review them you can see they clearly do not impact much else. Thanks again!

EDIT: Is there any value since I've forked and tested with +2.5yr codebase re-submit this small change in a new PR or will someone look at the old one? It sure seemed like given the comments with that PR were that it was tested as fixed it followed by a quick close, my guess is perhaps it was overlooked in actually merging, and is not actively being tested since it was thought to have been fixed?

@remitamine
Copy link
Collaborator

remitamine commented Mar 9, 2019

But it's been almost THREE YEARS and it's TEN lines added and two removed. I asked because it looks like it was just overlooked and I just tested and it appears to merge just fine with today's code so could you maybe try to push it into the active queue? The numbering is just broken as you can see, and these 10 lines fix it and if you review them you can see they clearly do not impact much else. Thanks again!

the change is done to an important file in the project and it's not isolated to a specific extractor, I won't blindly merge the changes because they work in one scenario and they might break other combination of options, and I'm not necessarily going be the person to review the pull request.

you might want to read https://github.com/rg3/youtube-dl#how-can-i-speed-up-work-on-my-issue.

@CollinChaffin
Copy link
Author

CollinChaffin commented Mar 10, 2019

Ok but it's absurd it actually is all of FOUR LINES OF CODE. FOUR. They impact nothing else and in THREE YEARS someone can't merge considering those 4 lines of code fix what is 100% a BROKEN counting/numbering? I appreciate your help since so much has changed in years the original PR cannot be merged as is but I have written a new one.

EDIT: Looks like I figured out the missing param issue I am working with a few diff versions of the branch and did the 2nd merge with the older one, LOL!

@CollinChaffin
Copy link
Author

After looking you actually marked this as duplicate of a closed issue that is not actually fixed. Shouldn't this be re-opened until such time that this bug is fixed? It's actually a bug not a feature to ask that the playlist index can maintain the correct index numbering upon a resume with specific list items.

@CollinChaffin CollinChaffin changed the title [Feature Request] True playlist index numbering option [BUG] Playlist index numbering is incorrect when using '--playlist-items' Mar 10, 2019
@remitamine
Copy link
Collaborator

After looking you actually marked this as duplicate of a closed issue

wrong, you can check the status of the issue, it's open.

@jxu
Copy link
Contributor

jxu commented Jan 28, 2020

@CollinChaffin if you submit a new PR maybe it will get looked at. who knows.

edit: in theory it is now a 1 line fix #23873

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants