-
Notifications
You must be signed in to change notification settings - Fork 12
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
Download media object #296
Conversation
For consistency with other scripts I recommend renaming Note also that rather than using TestConfiguration.json to set demov3 network you should be able to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, I made some suggestions, the only ones that I feel strongly about are:
- add a
--streamKey
argument (optionally defaulting to "video") - change
--objectVersion
to--versionHash
and--offeringType
to--offeringKey
for consistency with other scripts - use
default: ...
in option() declarations instead of setting the value via||
(this will make the default values show up automatically in help output when user is missing an option or specifies--help
)
I also suggest renaming ObjectDownloadMedia.js
to OfferingDownloadMedia.js
to reflect the fact that it targets a single mez offering.
testScripts/ObjectDownloadMedia.js
Outdated
throw new Error(`start or end time provided are not within range: ${totalDuration}s`); | ||
} | ||
|
||
const sourcesTimeInfo = sourcesMetadata.map((item) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommend renaming item
variable to part
or partInfo
for greater clarity
testScripts/ObjectDownloadMedia.js
Outdated
if(startTime < 0 || endTime > totalDuration) { | ||
throw new Error(`start or end time provided are not within range: ${totalDuration}s`); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional check?
- if startTime >= endTime throw...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly more checks,
- totalDuration < startTime < endTime, throw error
- startTime < totalDuration < endTime, throw warning but set endTime to the total Duration
testScripts/ObjectDownloadMedia.js
Outdated
}) | ||
.option("startTime", { | ||
describe: "start time to retrieve parts", | ||
type: "number" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
demandOption: true,
or
default: 0,
?
testScripts/ObjectDownloadMedia.js
Outdated
describe: "start time to retrieve parts", | ||
type: "number" | ||
}) | ||
.option("endTime", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
demandOption: true,
?
testScripts/ObjectDownloadMedia.js
Outdated
|
||
let objectId = this.args.objectId; | ||
const versionHash = this.args.objectHash; | ||
const offeringType = this.args.offeringType || "default"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can declare with default: "default",
in .option(...)
testScripts/ObjectDownloadMedia.js
Outdated
const offeringType = this.args.offeringType || "default"; | ||
const startTime = this.args.startTime; | ||
const endTime = this.args.endTime; | ||
const out = this.args.out || "./out"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can declare with default: "./out", in .option(...)
testScripts/ObjectDownloadMedia.js
Outdated
objectId, | ||
partHash | ||
}); | ||
let filePath = path.join(dirPath, partHash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to prepend an index so that e.g. instead of creating files:
hqpeKzJ5bBqKr38dEdNsX16epCwnqmwm5acnRcL13PvQkZhCnR9si
hqpe2NC2ojcbkPLrEbEKfFFSqroeST8Y2wgrJWRWtP61Lg7TfZ7nRT
the files wind up being
0001.hqpeKzJ5bBqKr38dEdNsX16epCwnqmwm5acnRcL13PvQkZhCnR9si
0002.hqpe2NC2ojcbkPLrEbEKfFFSqroeST8Y2wgrJWRWtP61Lg7TfZ7nRT
so that they are sorted in directory listings and user does not have to save part list from output to preserve ordering info.
Alternately you might be able to open a single file handle and write each part to same file handle in append mode, resulting in just one concatenated file being output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepended index to file name.
testScripts/ObjectDownloadMedia.js
Outdated
} | ||
|
||
|
||
const sourcesJsonPath = "offerings." + offeringType + ".media_struct.streams.video.sources[*]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to add an option --streamKey
that defaults to "video", and then statement would be "offerings." + offeringType + ".media_struct.streams." + streamKey + ".sources[*]";
Video stream is not guaranteed to be stored under the key "video", and (maybe?) a future user might want to download an audio track
Thanks for reviewing. Made the changes as per comment.
|
@elv-serban When attempting to trim a video, I have observed that if the duration is 19.7s, it is rounded down to 19s on my system, instead of rounding to 20s. However, the ffmpeg @eponymous301 is using rounds the duration up to 20s. I am not sure how to fix this. @elv-haoyu, could you please try the script when you have a moment? The |
As per @elv-haoyu requests, Made following changes:
|
testScripts/OfferingDownloadMedia.js
Outdated
} | ||
}; | ||
|
||
if(streamKey === "both") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function works well. A few action items to generalize the method:
- Support for Multiple Audio Streams
- Content object could have multiple audio streams, for instance
english_5_1, english_stereo, french_parisian_5_1, french_parisian_stereo
. - Allow users to specify multiple stream names in the --streamKey argument, separated by spaces (e.g., --streamKey "video english_stereo french_parisian_stereo").
- Check if the specified streams exist in the content metadata before downloading.
- Handling Duplicate Downloads
- When downloading the same content with the same start and end time but different streams, get an error
Directory already exists at /Users/jenniezhang/elvProjects/elv-client-js Error: Directory already exists at /Users/jenniezhang/elvProjects/elv-client-js/iq__3QhxBgWHZDkcN87irAYqf5hSGdya_00-01-09.820_00-02-04.820
, the method should:- Avoid errors due to existing directories.
- Allow downloading multiple streams for the same content without conflicts.
Example commands:
Download video: node ./testScripts/OfferingDownloadMedia.js --startTime 69.82 --endTime 124.82 --objectId=iq__3QhxBgWHZDkcN87irAYqf5hSGdya --config-url "https://main.net955305.contentfabric.io/config" --out . --streamKey video
Download audio: node ./testScripts/OfferingDownloadMedia.js --startTime 69.82 --endTime 124.82 --objectId=iq__3QhxBgWHZDkcN87irAYqf5hSGdya --config-url "https://main.net955305.contentfabric.io/config" --out . --streamKey "audio english_stereo french_parisian_stereo"
@elv-haoyu @eponymous301 @elv-serban made the changes as discussed. The details are here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test and looks good.
The script
OfferingDownloadMedia
performs the following:transcodes
orofferings
for each provided streamKey.Steps:
For instance, for
english_stereo
streamKey:english_stereo
folder => contains all the parts that fall within the start and end times.english_stereo.mp4
=> the concatenated fileenglish_stereo_trimeed.mp4
=> this file has been both concatenated and trimmed to the specified time range.Command usage: