Feature resize images #27

billybonks · 2022-05-28T13:37:17Z

Summary

Created a table for artworks in order to have flexibility in the amount of sizes and not have to add 2 or 3 columns per size.

for hash
for error

trigger checks is there any track that does not have an entry into the artwork table. artwork processing is atomic so either both get created or not.

IPFS Pinning trigger + processor has been built but not tested.

Deployment

Create a pr with the migration and merge it first
Install ImageMagic on EC2
add the new ENV variables
Merge this pr

State post deployment of this pr.

Different 700x700 and 200x200 sizes will be generated for all existing tracks, and they should be pinned

Next steps

would be to return a url with graphql and have a toggle in the app to see the experience with the new assets?

GA Release requirements

If we are happy with the results then we can decide if

do we want to store the original reference somewhere ?
Do we want to use the existing api or do we want to make a new api fro the assets
Pre render all urls for minor performance ?

package.json

billybonks · 2022-05-28T13:58:40Z

package.json

@@ -45,6 +46,9 @@
    "ethers": "^5.6.2",
    "graphql": "^16.3.0",
    "graphql-request": "^4.2.0",
+    "imagemagick": "^0.1.3",
+    "ipfs-api": "^26.1.2",
+    "ipfs-http-client": "^56.0.3",


i decided to use this just because its was easier for me, happy to use non sugar client, i see you mostly interact with axios

src/processors/default/processTrackArtworks.ts

billybonks · 2022-05-28T13:59:49Z

src/processors/default/processTrackArtworks.ts

+}
+
+async function uploadBuffer(buffer: Buffer) {
+  const url: any = '/ip4/127.0.0.1/tcp/5011'; // have to use any because create only accepts hash


this needs to change to env var, Do we have a write url in env variables?

sent to u directly

billybonks · 2022-05-28T14:00:18Z

src/processors/default/processTrackArtworks.ts

+  name: 'processTrackArtworks',
+  trigger: missingProcessedArtworks,
+  processorFunction: processorFunction,
+};


do we care about new lines at end of file, i can set a rule for it

whatevr, but yes am a fan of spam more linting rules

lol, i am a fan as well. what editor are you using btw?

src/triggers/ipfs.ts

billybonks · 2022-05-28T14:02:19Z

src/types/track.ts

@@ -18,6 +18,12 @@ export type NFTTrackJoin = {
  processedTrackId: string;
 }

+export type Artworks = {


i can make a new file for this type if required

np whichever

trackId and error seem missing?

also should be singular, just Artwork?

probably should be singular

musnit · 2022-05-30T22:06:20Z

src/db/migrations/20220528044344_processedArtworks.ts

@@ -0,0 +1,19 @@
+import { Knex } from 'knex';


i see u created this migration properly with timestamp - need to rename the other migrations to have timestamp too, as they're currently just in numeric order so could clash with timestamp ones - will do before merging this PR

haha ye i saw, was going to ask if i need to make mine the custom numbers

musnit · 2022-05-30T22:08:34Z

src/processors/default/processTrackArtworks.ts

+import { ProcessedTrack } from '../../types/track';
+import { rollPromises } from '../../utils/rollingPromises';
+
+const name = 'addMetadataIPFSHash';


Suggested change

const name = 'addMetadataIPFSHash';

unused?

musnit · 2022-05-30T22:24:56Z

src/processors/default/processTrackArtworks.ts

+    await callback(path);
+  })
+}
+// when i use ProcessedTrack it claims lossyArtworkURL does not exist :shrug:


yeh - its an optional field:

type ProcessedTrackArtwork = { lossyArtworkURL: string } | { lossyArtworkIPFSHash: string };

So the type claims that at least one of these fields will be there, but not necessarily both. E.g. There are some music nft platforms that only provide centralized artwork URL and no ipfs involvement, and there are other ones that only provide IPFS hash and no URL.

I think IPFS hash should take priority, so logic should be someth like:

If is IPFS hash, get image from ipfsClient.getHTTPURL(processedTrack.lossyArtworkIPFSHash);

If no IPFS hash, get image from processedTrack.lossyArtworkURL

Also, for the first case in the condition above to actually work, we'll require:

lossyArtworkIPFSHash has been successfully pinned

pinning has completed,

otherwise there is a race condition where this processTrackArtwork runs before the ipfsHash has completed pinning. anyway i think for now its fine, don't worry about this and if the race condition occurs it will just error and can clean errors+retry later

as the URL may not exist if for example its only on IPFS, in which case need to pull it from our IPFS gateway as in src/clients/ipfs.ts:getHTTPURL

hm this also means that pinning the

lossyArtworkIPFSHash has been successfully pinned
pinning has completed,

so check the database first then if its pinned use the hash otherwise the url.

my assumption for using ipfsHash first is because the the probability that ipfs will always exist as compared to the url is higher? or its a perf thing with the pin.

ipfsHash first prio is coz the hash is stored on chain and so more robust. whether the actual image is there or not is another question, but yes i think also more likely that the image exists too.

but ye ideally code should b able to handle all combinations of existing hash/ existing url or not, and existing actual file or not

can improve the edge cases it handles in future PR tho

musnit · 2022-05-30T22:26:50Z

src/processors/default/processTrackArtworks.ts

+  })
+}
+// when i use ProcessedTrack it claims lossyArtworkURL does not exist :shrug:
+const processArtwork = async function (clients: Clients, nft: any): Promise<void> {


Suggested change

const processArtwork = async function (clients: Clients, nft: any): Promise<void> {

const processArtwork = async function (clients: Clients, processedTrack: ProcessedTrack): Promise<void> {

(or maybe just name the var track if simpler, but nft is 100% misleading)

See above comment for why this codebase is being quite pedantic about types and rarely using any -> i think in most cases in this codebase, if it feels like Typescript is getting in the way of things, actually it's likely revealing some edge case u haven't thought of. Also don't be shy to keep the Typescript docs on hand while working with this codebase coz there are some uses of more esoteric features that require more robust understanding of Typescript.

ye this is true its mega misleading a bit of a legacy copy paste, ill fix the usage of the type :)

musnit · 2022-05-30T22:28:39Z

src/processors/default/processTrackArtworks.ts

+    let imageProcessingResults: any = null;
+    try {
+      const [largeImageBuffer, thumbnailImageBuffer]
+        = await Promise.all([resizeImage(originalPath, '700x700'), resizeImage(originalPath, '200x200')]);


maybe put the sizes in a constant config var at top of file?

musnit · 2022-05-30T22:30:22Z

src/processors/default/processTrackArtworks.ts

+        { error: e.toString(), size: 'thumbnail', trackId: nft.id }
+      ]
+    } finally {
+      await clients.db.insert(Table.processedArtworks, imageProcessingResults);


hmm so this will insert every processedArtworks row in a new DB request, one request per artwork, with all happening in parallel? not batched/super efficient but i think it's probably fine nonetheless?

I think it's super easy to batch i can do a return here and insert afterwards. My main goal was to get the insert into the db since i am using the non cursor approach.

musnit · 2022-05-30T22:34:17Z

src/processors/default/processTrackArtworks.ts

+  const buffer = await imageBuffer(nft.lossyArtworkURL);
+  return temporaryWrite(buffer, async function (originalPath) {
+    console.log(`wrote image to ${originalPath}`)
+    let imageProcessingResults: any = null;


Suggested change

let imageProcessingResults: any = null;

let imageProcessingResults: Artworks[] = [];

?

musnit · 2022-05-30T22:37:15Z

src/types/track.ts

@@ -18,6 +18,12 @@ export type NFTTrackJoin = {
  processedTrackId: string;
 }

+export type Artworks = {
+  id: number;
+  cid: string;


cid is optional (eg: if there's an error), also error is optional. but at least one of the two is required.

so to be super pedantic, could have a Union type of cidOrError = {cid:string } | {error:string }

and smth like

Artwork = { trackId: string; size: string; } & cidOrError

musnit · 2022-05-30T22:44:01Z

src/db/migrations/20220528044344_processedArtworks.ts

+export const up = async (knex: Knex) => {
+  console.log('Running create contracts bootstrap');
+  await knex.schema.createTable(Table.processedArtworks, (table: Knex.CreateTableBuilder) => {
+    table.increments('id');


hmm not sure if should have an increment id? trackId should be unique could just be used as the primary key. we do just use this in other places - don't think there are any autoincrementing ids anywhere else.

one of the motivation & requirements for no autoincrementing ids is to ensure that if we open source this and someone else runs it with their own endpoint, their DB looks the same and has the same ids as ours. With autoincrementing ids, this can't be guaranteed, for example consider each of the below scenarios:

we run the db

we add sound, catalog

we process sound tracks + catalog tracks on tuesday

on tuesday process sound tracks+catalog tracks from tuesday and we add noizd and process noizd tracks from monday and tuesday

on weds we process tracks from all 3

2 weeks later, someone else runs the thing:

they process all sound+catalog+noizd tracks from tuesday and wednesday

the other person will end up with a different order of IDs, since our initial run had the monday noizd tracks mixed in.

the current design has a nice property across all tables where the order of all operations in the pipeline is basically irrelevant and 2 different people running it will eventually converge on to mostly the same state and same db contents (with some minor buggy exceptions probably)

so this also means that if we add a peer-to-peer network at some point in future where different nodes share and sync their data, it will be easy to do so if everyone is on the same page about ordering, as there will be no need for debate or consensus between nodes on ordering.

(this motivation/requirement may not be that important tbh, could be a bit overkill/premature optimization, but given we've got this property already and it's easy to preserve for now, i think worth preserving)

ye i figured this would be a problem based on the other code that i read through :). was being a bit lazy. i cant use trackId since there are multiple images per track but i could follow your format that makes ids deterministic

ye i mean it may make sense to actually process images from the nft directly rather than from the track, and use the nft id too. would need to handle the case where multiple nfts all have the same image tho.

Track all transfers

Add sound cids

fix catalog zora tracks

billybonks force-pushed the feature/resize-images branch 2 times, most recently from 82cc0b3 to 4890997 Compare May 28, 2022 13:44

billybonks requested a review from musnit May 28, 2022 13:44

billybonks force-pushed the feature/resize-images branch 3 times, most recently from 579bdb4 to e0cfe7d Compare May 28, 2022 13:55

billybonks changed the title ~~Feature/resize images~~ Feature resize images May 28, 2022

billybonks changed the base branch from main to lint/autofix May 28, 2022 13:56

billybonks commented May 28, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

billybonks commented May 28, 2022

View reviewed changes

src/processors/default/processTrackArtworks.ts Show resolved Hide resolved

billybonks commented May 28, 2022

View reviewed changes

src/triggers/ipfs.ts Show resolved Hide resolved

billybonks commented May 28, 2022

View reviewed changes

billybonks force-pushed the feature/resize-images branch from 680e009 to da6ebc5 Compare May 28, 2022 14:03

musnit reviewed May 30, 2022

View reviewed changes

billybonks added 5 commits June 5, 2022 13:34

style: autofix lint

665807e

feat: create processTrackArtworks Processor

87da4b0

migration: create artworks table

5d24f84

feat: Pinner for processed artworks

a803391

feat: Add new processors to index

284c419

billybonks force-pushed the feature/resize-images branch from da6ebc5 to 284c419 Compare June 5, 2022 05:45

Base automatically changed from lint/autofix to main June 5, 2022 05:51

musnit added 10 commits June 12, 2022 18:18

track all transfers'

2164228

fix transfer tracker bug

f5d859e

new db dump

40f39de

update sound processor for new changes

e031e73

Merge pull request #1 from spinamp/track_all_transfers

ebef3e2

Track all transfers

fix sound api and add sound cids

e4ded14

save db

9d450e4

style fix

7564967

Merge pull request #2 from spinamp/add_sound_cids

3908691

Add sound cids

Merge branch 'main' into feature/resize-images

3c46a1c

musnit added a commit that referenced this pull request Aug 2, 2022

Merge pull request #27 from spinamp/nina_catalogfix

f92c797

fix catalog zora tracks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature resize images #27

Feature resize images #27

billybonks commented May 28, 2022 •

edited

Loading

billybonks May 28, 2022

billybonks May 28, 2022

musnit May 30, 2022

billybonks Jun 3, 2022

billybonks May 28, 2022

musnit May 30, 2022 •

edited

Loading

billybonks Jun 5, 2022

musnit Jun 7, 2022

billybonks May 28, 2022

musnit May 30, 2022

musnit May 30, 2022

billybonks Jun 3, 2022

musnit May 30, 2022

billybonks Jun 3, 2022

musnit May 30, 2022

musnit May 30, 2022

billybonks Jun 3, 2022

musnit Jun 3, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading

billybonks Jun 3, 2022

musnit May 30, 2022 •

edited

Loading

billybonks Jun 3, 2022

musnit May 30, 2022

billybonks Jun 3, 2022

musnit May 30, 2022

musnit May 30, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading

billybonks Jun 3, 2022

musnit Jun 3, 2022

	const processArtwork = async function (clients: Clients, nft: any): Promise<void> {
	const processArtwork = async function (clients: Clients, processedTrack: ProcessedTrack): Promise<void> {

	let imageProcessingResults: any = null;
	let imageProcessingResults: Artworks[] = [];

Feature resize images #27

Are you sure you want to change the base?

Feature resize images #27

Conversation

billybonks commented May 28, 2022 • edited Loading

Summary

Deployment

State post deployment of this pr.

Next steps

GA Release requirements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musnit May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musnit Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

musnit May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musnit May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musnit May 30, 2022 • edited Loading

Choose a reason for hiding this comment

musnit May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billybonks commented May 28, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading

musnit Jun 3, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading

musnit May 30, 2022 •

edited

Loading