Sound preference proposal, closes #83

ubershmekel commented 5 years ago

Thank you for doing this work! Two requests if you have time @smarek:

Seems to get v.reddit sounds to work we'd need to handle the hls_url instead of the fallback url. That's by looking at https://www.reddit.com/r/WhatsWrongWithYourDog/comments/bv7x95/so_our_dog_gets_a_bit_excited_when_asked_about/.json and https://www.reddit.com/r/WhatsWrongWithYourDog/comments/bv7x95/so_our_dog_gets_a_bit_excited_when_asked_about/
The buttons in the UI are getting too numerous and unwieldy. Something needs to be collapsed there perhaps. I also think I misplaced the comments and image links. All the checkboxes should be in one row. I don't expect you to to do that in this PR. Just saying I might have to do it if you fix the v.reddit issue.

smarek commented 5 years ago

@ubershmekel cool

1) fallback_url links directly the video, hls_url links m3u8 playlist, which is usually not very compatible with browsers, and I think we would have to integrate additional JS library to handle such playlists. Do you insist on replacing fallback_url with hls_url ?

This is what I see in media.reddit_video. block of JSON url you've linked, maybe the contents changes over time, and you've seen something very different?

"media": {
"reddit_video": {
"fallback_url": "https://v.redd.it/3fqsu5bczj131/DASH_720?source=fallback",
"height": 720,
"width": 1280,
"scrubber_media_url": "https://v.redd.it/3fqsu5bczj131/DASH_96",
"dash_url": "https://v.redd.it/3fqsu5bczj131/DASHPlaylist.mpd",
"duration": 61,
"hls_url": "https://v.redd.it/3fqsu5bczj131/HLSPlaylist.m3u8",
"is_gif": false,
"transcoding_status": "completed"
}
},

2) yes, the layout is not very nice, and i've got no idea what to do with it, however that is something I can fix easily, once we agree on what design we want to have

I propose this controls layout:

subreddit link | nswf | sound | fullscreen
hotkeys | src | blog | comments | image
auto next form seconds
pagination

what do you think?

ubershmekel commented 5 years ago

I'm not sure I understand your proposal. https://v.redd.it/3fqsu5bczj131/DASH_720?source=fallback has no audio as far as I can tell. Do you hear audio at that url?
For layout, I'm thinking of separating the types of UI. First row would be links, the second would be the checkboxes.

redditp-info | subreddit link | comments | image
nsfw | sound | auto next | duration | fullscreen
pagination

Then redditp-info would link to the github readme. This would remove hotkeys | src | blog which would be explained and linked at the github readme.

What do you think of that layout?

smarek commented 5 years ago

sorry, audio is available at fallback_url/audio see the script.js line 357 No audio is on the media links, from the dog link you posted, if you look in network trace, you can see reddit is loading DASH video of specific quality (720p) and audio separately
I like it, my proposal tried to keep some things where they were, but if you're cool with complete reorg, i like it :)

ubershmekel commented 5 years ago

This works on v.redd.it urls like https://www.reddit.com/r/WhatsWrongWithYourDog/comments/c3x7cc/doggo_thinks_that_he_is_one_of_them/.json

Does not work on gfycat urls like https://www.reddit.com/r/nextfuckinglevel/comments/c3v7b3/looks_so_effortless_but_he_must_have_trained_very/.json

ubershmekel commented 4 years ago

I re-merged and tested this. This works for a file that has video+audio in it, but not for separate video and audio files like reddit videos. Seems only imgur and gfycat will work with this.

I think to make it fully work we'd need to implement something like this JS multiplexing. Which is what I assume reddit video has done: https://medium.com/canal-tech/how-video-streaming-works-on-the-web-an-introduction-7919739f7e1

I'm not sure if we should merge it in when reddit videos don't work.

smarek commented 4 years ago

Could you maybe create a branch on which we could propose PRs to fix remaining issues with sound, from mentioned "re-merged" branch you already have? Or if you have staging environment somewhere for others to test the sound with (if not, it can be disabled by default and listed under experimental options)

Also the "multiplexing" is imho not needed, as you can simply provide different URL that contains MP4 container (audio+video) , or provide separate <video and <audio html tags and sync them using https://stackoverflow.com/a/32323192/492624 or similar techniques

ubershmekel commented 4 years ago

https://github.com/ubershmekel/redditp/commits/smarek-sound2

You're right. Making separate elements would work too.

smarek commented 4 years ago

@ubershmekel before i start working on that, would you be cool with using dash.js for playing (json)reddit_video.dash_url instead of using plain html5 elements?

ubershmekel commented 4 years ago

I see the minified file is 577 KB (non-gzipped size). I couldn't find a browser compatibility matrix. Can you explain the cost-benefit? Seems a bit heavy and potentially fragile.

smarek commented 4 years ago

Given raw-json

"reddit_video": {
"fallback_url": "https://v.redd.it/iysd6l20k3941/DASH_480?source=fallback",
"height": 480,
"width": 384,
"scrubber_media_url": "https://v.redd.it/iysd6l20k3941/DASH_96",
"dash_url": "https://v.redd.it/iysd6l20k3941/DASHPlaylist.mpd",
"duration": 19,
"hls_url": "https://v.redd.it/iysd6l20k3941/HLSPlaylist.m3u8",
"is_gif": false,
"transcoding_status": "completed"
}

We can either do 1) Plain html+js (whatever source data) 2) dash.js (dash_url) 3) hls.js (hls_url) 4) ??? 5) (maybe we can do all of them, and fall-back through when given browser does not support one but supports other?)

I've found conflicted sources, but it seems to me that HLS, being Apple technology, has far more lower support than DASH

However reddit itself (on my device) opens with HLS source

<video poster="https://external-preview.redd.it/SZJwCdXt4knnZgskQuE2_C_QDyCXQeDNReuwC-lp5aw.png?width=320&amp;crop=smart&amp;format=pjpg&amp;auto=webp&amp;s=53e709f966bf326c52cb3b64b995cdb2525e2bd8" muted="" preload="auto" class="_1EQJpXY7ExS04odI1YBBlj" src="blob:https://www.reddit.com/b59eb125-a56e-46b4-9d40-9bd09d38b4c4">
<source src="https://v.redd.it/iysd6l20k3941/HLSPlaylist.m3u8" type="application/vnd.apple.mpegURL">
</video>

I'd be really grateful if someone could investigate the protocol compatibility between HLS/DASH

Relevant tickets

https://github.com/Dash-Industry-Forum/dash.js/issues/2055

smarek commented 4 years ago

@ubershmekel I'm quite sorry, but I've solved this ticket and related #104 ticket, but the PR is quite a lot changes, because I was not able to develop efficiently, with so much code issues.

Changes i've included along (and the PR is currently mergable without conflicts against master)

ignoring idea (idea-community/webstorm/phpstorm) dev files
code formatting, terminating statements with ;, strict comparison in if statements
ability to handle single-post URL (useful for debugging)
fixed distinguish for json/jsonp requests
disable cloud reporting on non-production environment

I've left the commits un-squashed, but after review, if accepted, i can (or you can) squash some of them together

Also, since I've updated this PR and did not create new one against branch smarek-sound2, said branch can be deleted currently

smarek commented 4 years ago

Cool, deploy preview works on my end correctly in all test-cases, check the links below:

https://deploy-preview-84--redditp.netlify.com/ https://deploy-preview-84--redditp.netlify.com/r/all https://deploy-preview-84--redditp.netlify.com/r/WhatsWrongWithYourDog/comments/c3x7cc/doggo_thinks_that_he_is_one_of_them https://deploy-preview-84--redditp.netlify.com/r/nextfuckinglevel/comments/c3v7b3/looks_so_effortless_but_he_must_have_trained_very https://deploy-preview-84--redditp.netlify.com/r/random https://deploy-preview-84--redditp.netlify.com/r/randnsfw https://deploy-preview-84--redditp.netlify.com/

ubershmekel commented 4 years ago

Looks like a lot of work. What's the state of affairs?

smarek commented 4 years ago

What do you mean?

smarek commented 4 years ago

If the single proposed change is everything, i think, it's ready to merge

ubershmekel commented 4 years ago

Looking at the code, this is all great stuff. I wish I had a test suite that could run through more scenarios. I hope to finish reviewing tomorrow.

smarek commented 4 years ago

Only thing I'm not satisfied with, is the detection of subreddits, that redirect.

There are certainly more than what's hard-coded currently, and I did not find any official list of this kind of subreddits, and detecting whether the call resulted in response from different subreddit (which might not be complete/best solution), is not possible via standard jQuery XHR means (currently being solved here https://github.com/jquery/jquery/pull/4405 )

I propose to solve this in subsequent PR, because ie. r/random returns data from randomly picked subreddit, but currently implementation calls next page of items from url such as http://www.reddit.com/r/random.json?&after=t3_fap561& which is not correct obviously

ubershmekel commented 4 years ago

I love that getting a /comments/ URL works now. I was hoping to get that fixed some day. I'm curious why you need to toggle between jsonp and not for specific URLs. I thought JSONP works for everything. No?

Merging. We'll get the rest sorted later. Thank you so much for all this work @smarek it works beautifully as far as I can tell.

smarek commented 4 years ago

I love that getting a /comments/ URL works now. I was hoping to get that fixed some day. I'm curious why you need to toggle between jsonp and not for specific URLs. I thought JSONP works for everything. No?

This is given by response type of the endpoint, when the request is redirected or single-post, the response type does not match application/json, and is blocked by CORB (at least in my chrome dev version 82)

Anyway, I'm glad you like the work, and you should probably delete the smarek-sound2 branch now

ubershmekel / redditp

Sound preference proposal, closes #83 #84